Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix delete updates #6194
Fix delete updates #6194
Changes from 3 commits
51d1535
40e9df3
53a52fb
706f3d6
8b73fbd
c6c27d2
41cffbc
942a64e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Adding this config to the compactor looks out of place to me. New users already get confused by our configs, so it is better to duplicate it a little than add to the confusion. It could be just me. What do you think?
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 went back and forth on this. If we leave it off the compactor config, it becomes a required property on:
Do you think that's clearer for the user than having the compactor know it's own address?
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.
What do you think about making it part of
storage
config? It is anyways something related to storage.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.
is there also a case to be made for the
common
config, since it's common across multiple components?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.
Common Config says
// Values defined under this common configuration are supersede if a more specific value is defined.
Does that imply that there's also some more specific override elsewhere? If not, it would be a fine place for this to go.
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 don't think that would apply in this case, if we moved it to the common config so that was the only place it's defined, then there would be no superseding. What that comment is meant to convey is that after things from the common config (like storage or ring configs) are applied to the many places those are used, the
config.yaml
is read again, so if there is say, a specific ring config for the scheduler, that will supersede whatever was put in common.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.
yeah, I thought about using
common
config but due to that comment I thought it was not the right place, same as Travis. If we can use it for this use case then it would be a better option.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.
Instead of taking an HTTP client as a parameter, what do you think about accepting a config?
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'd prefer to take the client, if it's not too disruptive. I really like this pattern because it makes if very easy to unit test without worrying about the network: pkg/storage/stores/shipper/compactor/deletion/delete_requests_client_test.go