[History Sever] Use compression library in collector and historyserver#4899
[History Sever] Use compression library in collector and historyserver#4899chiayi wants to merge 2 commits into
Conversation
chiayi
force-pushed
the
history-server-compress
branch
from
6eade58 to
cf8f7a8
Compare
chiayi
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf8f7a8aca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| sessionPath := path.Clean(path.Join(ec.root, utils.AppendRayClusterNameNamespace(ec.clusterName, ec.clusterNamespace), sessionNameToUse)) | ||
|
|
||
| basePath := path.Join(sessionPath, "node_events", fmt.Sprintf("%s-%s", nodeIDToUse, hourKey)) | ||
| basePath := path.Join(sessionPath, "node_events", fmt.Sprintf("%s-%s.gz", nodeIDToUse, hourKey)) |
There was a problem hiding this comment.
When the collector starts writing node_events with a .gz suffix here, the dead-cluster log path that accepts node_ip can no longer resolve the node ID: ServerHandler.ipToNodeId still lists these files and searchNodeIDHexInEventFile reads them via GetContent and passes the compressed bytes directly to json.Unmarshal (historyserver/pkg/historyserver/reader.go:522-537). In environments where users call /logs with node_ip instead of node_id after collection, this will skip every newly written node event file and return node_id not found; that reader needs the same gzip-aware path as ProcessSingleSession.
Useful? React with 👍 / 👎.
chiayi
force-pushed
the
history-server-compress
branch
from
cf8f7a8 to
e29adaa
Compare
| } else { | ||
| // GetContent and io.ReadAll failures are treated as transient storage errors: | ||
| // skip this file and continue. | ||
| eventioReader = h.reader.GetContent(clusterNameNamespace, eventFile) | ||
| } |
There was a problem hiding this comment.
Do we still need this branch, given that we changed eventFilePattern to match only the .gz suffix?
There was a problem hiding this comment.
I originally kept this branch for backwards compatibility, but I'm guessing we might not have to worry about that given the where the project is at currently, will instead just add a warning that file is not supported.
There was a problem hiding this comment.
Actually, will change it so that there is a path to support uncompressed event files as well.
chiayi
force-pushed
the
history-server-compress
branch
from
e29adaa to
81e61f9
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 81e61f9. Configure here.
chiayi
force-pushed
the
history-server-compress
branch
2 times, most recently
from
f4e4062 to
5660bc5
Compare
chiayi
force-pushed
the
history-server-compress
branch
from
5660bc5 to
895293c
Compare
|
|
|
@rueian Looked into the test failures. One of them is due to the test not being wrapped in the compression library reader. Specifically the |
3 participants
Why are these changes needed?
This PR is for utilizing the compression library from #4828 in the collector and historyserver components
Related issue number
#4804
Checks