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/15 19:37:49 UTC
[GitHub] [lucene-solr] HoustonPutman commented on a change in pull request #1861: SOLR-10391: Add overwrite option to UPLOAD ConfigSet action
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