-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
SOLR-10391: Add overwrite option to UPLOAD ConfigSet action #1861
Conversation
I wish we followed REST approaches for so many of our API's... Each one has a slightly different semantics for basic CRUD. Wish we had:
|
I agree with @epugh on the whole REST thing. I also think that the default behavior should be that files that aren't present in the new configSet should be removed. I can see the use case for keeping old files around, but I think most people would want to update all files together. I could be wrong about that though. |
I wish that too, but Solr APIs aren't REST as of today (not even V2 API is fully REST, and this was intentional AFAIK, things like
I was planning to add that as a different PR but also making it not-default, mostly because it can be pretty destructive (i.e. you created the zip file incorrectly, or created it with the "conf" directory on top or something like that it could remove all files) but I'm fine if people think it should be the default. Also, are you OK with doing that as a different PR or want me to do the change here? My intention was to keep the PR small for easier reviews, but if we think this feature is incomplete without that I can do it here too, I'm fine either way. |
if (zipEntry.isDirectory()) { | ||
zkClient.makePath(filePathInZk, true); | ||
zkClient.makePath(filePathInZk, false, true); |
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.
We might want to clear data that the znode has if it exists and has data.
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.
Can that ever happen in a configset?
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.
Not very likely, was just thinking about possibilities. It's probably not an issue 99.9% of times, so it likely doesn't need to be addressed.
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.
Yes, I don't think this would be needed at this point. No API would set this, and no Solr component would read it. I guess the only case would be a custom component that writes and reads directly to ZooKeeper.
solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
Outdated
Show resolved
Hide resolved
5e97343
to
879c880
Compare
I plan to merge this tomorrow |
When set to 'true', Solr will override an existing configset in ZooKeeper if an UPLOAD action happens on an existing configset
f205a56
to
add38ed
Compare
When set to true, Solr will overwrite an existing configset in ZooKeeper in an UPLOAD. A new cleanup parameter can also be passed to let Solr know what to do with the files that existed in the old configset, but no longer exist in the new configset (remove or keep)
When set to
true
, Solr will overwrite an existing configset in ZooKeeper if an UPLOAD action happens on an existing configset.A new
cleanup
parameter can also be passed to let Solr know what to do with the files that existed in the old configset, but no longer exist in the new configset (remove or keep)