You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/09/12 00:23:38 UTC
[GitHub] [lucene-solr] tflobbe opened a new pull request #1861: SOLR-10391: Add override option to UPLOAD ConfigSet action
tflobbe opened a new pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861
When set to `true`, Solr will override an existing configset in ZooKeeper if an UPLOAD action happens on an existing configset
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#issuecomment-691496341
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe commented on pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#issuecomment-696456676
I plan to merge this tomorrow
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#discussion_r489001572
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
Review comment:
As it currently stands, an overwrite never updates the "trusted" flag, if it wasn't trusted before, it'll continue to be untrusted after the upgrade. If it was trusted, only an authenticated request would be allowed. I see your point that if "cleanup" is set to "true", an authenticated request could turn the configset to trusted, is that what you are suggesting?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#issuecomment-691496341
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on a change in pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#discussion_r488913454
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length() -1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
Review comment:
We might want to clear data that the znode has if it exists and has data.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
Review comment:
If you are overwriting an untrusted configSet with a trusted configSet, I din't think that will get updated here. I guess you would only want it if CLEANUP is true, since only then are you sure that all of the content is trusted.
So doing it in that if statement would probably work.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length() -1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
} else {
createZkNodeIfNotExistsAndSetData(zkClient, filePathInZk,
IOUtils.toByteArray(zis));
}
}
zis.close();
+ deleteUnusedFiles(zkClient, filesToDelete);
+ }
+
+ private void deleteUnusedFiles(SolrZkClient zkClient, Set<String> filesToDelete) throws InterruptedException, KeeperException {
+ if (!filesToDelete.isEmpty()) {
+ if (log.isInfoEnabled()) {
+ log.info("Cleaning up {} unused files", filesToDelete.size());
+ }
+ if (log.isDebugEnabled()) {
+ log.debug("Cleaning up unused files: {}", filesToDelete);
+ }
+ for (String f:filesToDelete) {
+ try {
+ zkClient.delete(f, -1, true);
+ } catch (KeeperException.NoNodeException nne) {
+ }
+ }
+ }
+ }
+
+ private Set<String> getAllConfigsetFiles(SolrZkClient zkClient, String configPathInZk) throws KeeperException, InterruptedException {
+ final Set<String> files = new HashSet<>();
+ if (!configPathInZk.startsWith(ZkConfigManager.CONFIGS_ZKNODE + "/")) {
+ throw new IllegalArgumentException("\"" + configPathInZk + "\" not recognized as a configset path");
+ }
+ ZkMaintenanceUtils.traverseZkTree(zkClient, configPathInZk, ZkMaintenanceUtils.VISIT_ORDER.VISIT_POST, new ZkMaintenanceUtils.ZkVisitor() {
+ @Override
+ public void visit(String path) throws InterruptedException, KeeperException {
+ files.add(path);
+ }
+ });
+ files.remove(configPathInZk);
+ return files;
+ }
+
+ /*
+ * Fail if an untrusted request tries to update a trusted ConfigSet
+ */
+ private void ensureOverwritingUntrustedConfigSet(SolrZkClient zkClient, String configSetZkPath) {
+ byte[] configSetNodeContent;
+ try {
+ configSetNodeContent = zkClient.getData(configSetZkPath, null, null, true);
+ } catch (KeeperException e) {
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Exception while fetching current configSet at " + configSetZkPath, e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Interrupted while fetching current configSet at " + configSetZkPath, e);
+ }
+ @SuppressWarnings("unchecked")
+ Map<Object, Object> contentMap = (Map<Object, Object>) Utils.fromJSON(configSetNodeContent);
+ Boolean isCurrentlyTrusted = (Boolean) contentMap.get("trusted");
Review comment:
using `getOrDefault("trusted", false)` would remove the need for the null check.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length() -1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
} else {
createZkNodeIfNotExistsAndSetData(zkClient, filePathInZk,
IOUtils.toByteArray(zis));
}
}
zis.close();
+ deleteUnusedFiles(zkClient, filesToDelete);
+ }
+
+ private void deleteUnusedFiles(SolrZkClient zkClient, Set<String> filesToDelete) throws InterruptedException, KeeperException {
+ if (!filesToDelete.isEmpty()) {
+ if (log.isInfoEnabled()) {
+ log.info("Cleaning up {} unused files", filesToDelete.size());
+ }
+ if (log.isDebugEnabled()) {
+ log.debug("Cleaning up unused files: {}", filesToDelete);
+ }
+ for (String f:filesToDelete) {
+ try {
+ zkClient.delete(f, -1, true);
+ } catch (KeeperException.NoNodeException nne) {
+ }
+ }
+ }
+ }
+
+ private Set<String> getAllConfigsetFiles(SolrZkClient zkClient, String configPathInZk) throws KeeperException, InterruptedException {
+ final Set<String> files = new HashSet<>();
+ if (!configPathInZk.startsWith(ZkConfigManager.CONFIGS_ZKNODE + "/")) {
+ throw new IllegalArgumentException("\"" + configPathInZk + "\" not recognized as a configset path");
+ }
+ ZkMaintenanceUtils.traverseZkTree(zkClient, configPathInZk, ZkMaintenanceUtils.VISIT_ORDER.VISIT_POST, new ZkMaintenanceUtils.ZkVisitor() {
+ @Override
+ public void visit(String path) throws InterruptedException, KeeperException {
+ files.add(path);
+ }
+ });
Review comment:
the `new ZKMaintenanceUtils.ZKVisitory() { ... }` can be replaced with `files::add`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#discussion_r492423507
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length() -1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#discussion_r492423507
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length() -1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe commented on pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#issuecomment-692195569
> I wish we followed REST approaches for so many of our API's
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](https://issues.apache.org/jira/browse/SOLR-8029?focusedCommentId=14740121&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14740121), things like `MODIFYCOLLECTION` in V2 API are still POSTs requests). I think I'd rather keep consistency with other APIs than make this particular one better?
> I also think that the default behavior should be that files that aren't present in the new configSet should be removed.
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on a change in pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#discussion_r489447516
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length() -1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] epugh commented on pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#issuecomment-691496341
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:
```
CRUD HTTP
Create POST
Read GET
Update PUT
Delete DELETE
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#issuecomment-692080290
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe commented on pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#issuecomment-696456676
I plan to merge this tomorrow
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#discussion_r492423507
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length() -1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe merged pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe merged pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#discussion_r489001982
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length() -1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
Review comment:
Can that ever happen in a configset?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] HoustonPutman commented on a change in pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#discussion_r489445830
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " + Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
Review comment:
Yeah, for example if you want to use plugins or something similar, I could see someone not using a trusted configSet at first, but then wanting to overwrite their configSet to be "trusted" to be able to use the plugins.
I don't see why that shouldn't be possible, given that there's little difference security-wise between creating a new config set and overwriting w/ cleanup.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe merged pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe merged pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
Posted by GitBox <gi...@apache.org>.
tflobbe commented on pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#issuecomment-696456676
I plan to merge this tomorrow
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org