Skip to content

Conversation

@JiangJiaWei1103
Copy link
Contributor

@JiangJiaWei1103 JiangJiaWei1103 commented Jan 1, 2026

Why are these changes needed?

This PR focuses on non-functional cleanup to improve readability, consistency, and maintainability.

Changes

Related issue number

#4304.

NOTE: This PR addresses all review comments, except for the docs part.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@JiangJiaWei1103 JiangJiaWei1103 changed the title [Refactor] [history server] [collector] Improve collector code structure and clarity [Refactor] [history server] [collector] Fix collector 4241 review comments Jan 2, 2026
@JiangJiaWei1103 JiangJiaWei1103 moved this to Work in progress in My Kuberay & Ray Jan 2, 2026
@JiangJiaWei1103 JiangJiaWei1103 changed the title [Refactor] [history server] [collector] Fix collector 4241 review comments [history server] [collector] Fix collector 4241 review comments Jan 3, 2026
Signed-off-by: JiangJiaWei1103 <[email protected]>
Comment on lines +23 to +24
CreationTime string `json:"creationTime"`
CreationTimestamp int64 `json:"creationTimestamp"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think CreationTime is more human-friendly and can be displayed directly in the UI. In contrast, CreationTimestamp is more machine-oriented and better suited for sorting and comparisons, for example:

func (a ClusterInfoList) Less(i, j int) bool { return a[i].CreateTimeStamp > a[j].CreateTimeStamp } // 降序排序

If this feels redundant, we can discuss which one to keep to improve maintainability.

Original discussion: #4241 (comment)


// OSS meta file keys used by history server.
const (
RayMetaFile_BasicInfo = "ack__basicinfo"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to clarify where these env vars will be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant