-
Notifications
You must be signed in to change notification settings - Fork 194
[Breaking] Change bytestream to array-based config #1848
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
base: main
Are you sure you want to change the base?
Conversation
ec97b4d
to
4804332
Compare
4804332
to
9b15eb4
Compare
9b15eb4
to
9254721
Compare
Please just resolve the conflicts in the original PR. |
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.
The previous PR didn't have it, but I think we should have the backwards compatibility in there
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.
Changes here should be reverted
_timeout_streaam_drop_guard: spawn!("bytestream_idle_stream_timeout", async move { | ||
(*sleep_fn)().await; | ||
_timeout_stream_drop_guard: spawn!("bytestream_idle_stream_timeout", async move { | ||
sleep(timeout).await; |
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.
Why the change? Doesn't feel related to the other bytestream work?
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.
I agree
|
||
let store_config = self | ||
.get_store_config(instance_name) | ||
.err_tip(|| format!("'instance_name' not configured for '{instance_name}'"))?; |
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.
get_store_config
already does this error
|
||
let store_config = self | ||
.get_store_config(&instance_name) | ||
.err_tip(|| format!("'instance_name' not configured for '{instance_name}'"))?; |
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.
get_store_config
already does this error
@@ -42,7 +39,7 @@ memory and filesystem stores instead of S3 and Redis. | |||
"temp_path": "/tmp/nativelink/data-worker-test/tmp_path-cas", | |||
"eviction_policy": { | |||
// 10gb. |
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.
Comment can go away now it's written out
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.
agree
}, | ||
"OSFamily": { | ||
"values": [""] | ||
}, | ||
"container-image": { | ||
"values": [""] | ||
}, | ||
// Example of how to set which docker images are available and set |
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.
Why the large docs changes?
1fb3bcc
to
40bd484
Compare
In contrast to TraceMachina#1712, there's no backwards-compatibility layer. Closes TraceMachina#1649 Fix the merge conflicts for the Bytestream config JSON file
40bd484
to
e38ff56
Compare
This is a continuation of the previously opened PR to change the bytestream to array-based config.
In contrast to #1712, there's no backwards-compatibility layer.
Closes #1649
Description
Please include a summary of the changes and the related issue. Please also
include relevant motivation and context.
Fixes # (issue)
Type of change
Please delete options that aren't relevant.
not work as expected)
How Has This Been Tested?
Please also list any relevant details for your test configuration
Checklist
bazel test //...
passes locallygit amend
see some docsThis change is
Co-authored by @jaroeichler