feat: Add deregister_object_store
#17999
Conversation
github-actions
bot
added
core
|
|
||
| /// Deregister the store previously registered with the same key. Returns the | ||
| /// deregistered store if it existed. | ||
| fn deregister_store(&self, url: &Url) -> Result<Arc<dyn ObjectStore>>; |
There was a problem hiding this comment.
This is breaking, we can add a label. I'm thinking maybe we can have a default return value and remove it in another version but I don't know what would be a good default return value (I was thinking of just erroring).
There was a problem hiding this comment.
Do you mean that giving it a default value and letting users be aware of the new API in the next release, then removing the default value in the next release after next?
There was a problem hiding this comment.
Yes, but I am very open to doing it another way if there is a more standard way to deal with this.
There was a problem hiding this comment.
I suggest a good return value would be returning an unimplemented error
That would be backwards compatible and non API breaking
There was a problem hiding this comment.
Made the changes
|
@alamb Would you like to take a quick look? |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @jonathanc-n -- I left a suggestion for the default. Otherwise looks good to me
|
|
||
| /// Deregister the store previously registered with the same key. Returns the | ||
| /// deregistered store if it existed. | ||
| fn deregister_store(&self, url: &Url) -> Result<Arc<dyn ObjectStore>>; |
There was a problem hiding this comment.
I suggest a good return value would be returning an unimplemented error
That would be backwards compatible and non API breaking
|
@jonathanc-n I'm so sorry, I missed the PR. Good job! /cc @zhuqi-lucas , the PR is merged |
|
Nice, thank you @jonathanc-n @alamb @xudong963 ! I will do follow-up in our internal project. |
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#17854 . ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
4 participants
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?