Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Java] make java worker log file prefix (default to java-worker) configurable #33797

Merged
merged 3 commits into from
Apr 10, 2023

Conversation

jiafuzha
Copy link
Contributor

@jiafuzha jiafuzha commented Mar 28, 2023

Why are these changes needed?

For java worker, it's log file always being prefixed with "java-worker". And in python log_monitor.py, it hardcodes "java-worker*.log" to be polled for new log msg periodically. Some configs, like log_to_driver and RAY_BACKEND_LOG_LEVEL, don't prevent the log monitor from polling and publishing logs to gcs. To save some CPU cycle and network bandwidth, especially if there is large amount of logs produced from JVM, we can have an option. like a JVM system property, to set log file prefix for java worker instead of hard coded to "java-worker".

Related issue number

Closes #33787

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@jiafuzha
Copy link
Contributor Author

@jovany-wang @kira-lin please help review.

@simon-mo
Copy link
Contributor

This is going to break log monitor, please make the corresponding change there as well:

log_file_paths = glob.glob(f"{self.logs_dir}/worker*[.out|.err]") + glob.glob(
f"{self.logs_dir}/java-worker*.log"
)

@jiafuzha
Copy link
Contributor Author

This is going to break log monitor, please make the corresponding change there as well:

log_file_paths = glob.glob(f"{self.logs_dir}/worker*[.out|.err]") + glob.glob(
f"{self.logs_dir}/java-worker*.log"
)

As tested, log monitor worked well with either property "ray.logging.file-prefix" set to "raydp-java-worker" or unset and default to "java-worker".

Could you please be more specific why log_monitor is breaking? From code, 'glob.glob(f"{self.logs_dir}/java-worker*.log"' returns empty list if there is not java-worker* files.

thanks

@jovany-wang
Copy link
Contributor

@zjf2012 So your purpose is to prevent polling java worker log messages to GCS right?

@jiafuzha
Copy link
Contributor Author

@zjf2012 So your purpose is to prevent polling java worker log messages to GCS right?

exactly. And it's configurable.

@@ -89,6 +89,7 @@ ray {
max-file-size: 500MB
// Maximum number of backup files to keep around.
max-backup-files: 10
file-prefix: java-worker
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment on what this field is.

Copy link
Contributor

@jovany-wang jovany-wang left a comment

Choose a reason for hiding this comment

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

LGTM.

@jovany-wang
Copy link
Contributor

Hi @simon-mo , I don't think this breaks anything. Maybe also @rkooo567 have a double check.

@jovany-wang jovany-wang added the do-not-merge Do not merge this PR! label Mar 29, 2023
@jovany-wang
Copy link
Contributor

DO NOT MERGE until 2.4 branch cut.

@jovany-wang
Copy link
Contributor

@zjf2012 do you need this feature in Ray2.4?

@simon-mo
Copy link
Contributor

Ah. This is a hacky way to not to stream log through log monitor it seems. Alternatively an env var in log monitor yo flag this will do.

@jiafuzha
Copy link
Contributor Author

@zjf2012 do you need this feature in Ray2.4?

yes, we need it in ray 2.4.

By the way, why do we need to wait branch 2.4 before merging it?

@jovany-wang
Copy link
Contributor

@simon-mo If we don't have the need that preventing polling only Java log, it makes more sense to add a flag to logmonitor.

@jovany-wang
Copy link
Contributor

@zjf2012 Because we changed the releasing process. We shouldn't merge unnecessary changes when in the process of releasing duration.

yes, we need it in ray 2.4.

@zhe-thoughts CC

@zhe-thoughts
Copy link
Collaborator

I'd like @simon-mo to confirm about his open question first. Thanks for working on this @jovany-wang

@simon-mo
Copy link
Contributor

With the comments added, the change LGTM

@jovany-wang jovany-wang added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed do-not-merge Do not merge this PR! labels Apr 10, 2023
@jovany-wang jovany-wang merged commit 10861d9 into ray-project:master Apr 10, 2023
@jovany-wang
Copy link
Contributor

Thanks for the contribution! This will be in the next release.

elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
…igurable (ray-project#33797)

For java worker, it's log file always being prefixed with "java-worker". And in python log_monitor.py, it hardcodes "java-worker*.log" to be polled for new log msg periodically. Some configs, like log_to_driver and RAY_BACKEND_LOG_LEVEL, don't prevent the log monitor from polling and publishing logs to gcs. To save some CPU cycle and network bandwidth, especially if there is large amount of logs produced from JVM, we can have an option. like a JVM system property, to set log file prefix for java worker instead of hard coded to "java-worker".

Signed-off-by: elliottower <[email protected]>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
…igurable (ray-project#33797)

For java worker, it's log file always being prefixed with "java-worker". And in python log_monitor.py, it hardcodes "java-worker*.log" to be polled for new log msg periodically. Some configs, like log_to_driver and RAY_BACKEND_LOG_LEVEL, don't prevent the log monitor from polling and publishing logs to gcs. To save some CPU cycle and network bandwidth, especially if there is large amount of logs produced from JVM, we can have an option. like a JVM system property, to set log file prefix for java worker instead of hard coded to "java-worker".

Signed-off-by: Jack He <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Java] make java worker log file prefix (default to java-worker) configurable
4 participants