-
Notifications
You must be signed in to change notification settings - Fork 7k
[core][event] feature flag for enabling ray export event #57999
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adds a feature flag to control the start of Ray event exporting. My review points out a potential for unnecessary resource usage when event exporting is disabled and suggests a follow-up improvement to address it.
df575c5 to
17f7d56
Compare
17f7d56 to
ac3a2a7
Compare
| } | ||
| } | ||
|
|
||
| bool RayEventRecorder::isEnabled() { return RayConfig::instance().enable_ray_event(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the method is unnecessary -- it's only used internally and it's a one liner, so I'd just inline the config check
edoakes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm aside from other nit. also: does it need to be cherry picked?
Signed-off-by: Cuong Nguyen <[email protected]>
ac3a2a7 to
8a2f851
Compare
…#57999) We have a feature flag to control the rolling out of ray export event, but the feature flag is missing the controlling of `StartExportingEvents`. This PR fixes the issue. Test: - CI Signed-off-by: Cuong Nguyen <[email protected]> Signed-off-by: xgui <[email protected]>
…#57999) We have a feature flag to control the rolling out of ray export event, but the feature flag is missing the controlling of `StartExportingEvents`. This PR fixes the issue. Test: - CI Signed-off-by: Cuong Nguyen <[email protected]>
…#57999) We have a feature flag to control the rolling out of ray export event, but the feature flag is missing the controlling of `StartExportingEvents`. This PR fixes the issue. Test: - CI Signed-off-by: Cuong Nguyen <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>
We have a feature flag to control the rolling out of ray export event, but the feature flag is missing the controlling of
StartExportingEvents. This PR fixes the issue.Test: