You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "fabriziofortino (via GitHub)" <gi...@apache.org> on 2023/02/08 10:00:21 UTC
[GitHub] [jackrabbit-oak] fabriziofortino commented on a diff in pull request #847: OAK-10098 | Support Index purge for ES indexes
fabriziofortino commented on code in PR #847:
URL: https://github.com/apache/jackrabbit-oak/pull/847#discussion_r1099880952
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/indexversion/IndexVersionOperation.java:
##########
@@ -90,25 +93,28 @@ public static List<IndexVersionOperation> generateIndexVersionOperationList(Node
* @return This method returns an IndexVersionOperation list i.e indexNameObjectList marked with operations
*/
public static List<IndexVersionOperation> generateIndexVersionOperationList(NodeState rootNode, String parentPath,
- List<IndexName> indexNameObjectList, long purgeThresholdMillis, boolean shouldPurgeBaseIndex) {
+ List<IndexName> indexNameObjectList, long purgeThresholdMillis, boolean shouldPurgeBaseIndex, boolean ignoreIsIndexActiveCheck) {
NodeState indexDefParentNode = NodeStateUtils.getNode(rootNode, parentPath);
List<IndexName> reverseSortedIndexNameList = getReverseSortedIndexNameList(indexNameObjectList);
List<IndexVersionOperation> indexVersionOperationList = new LinkedList<>();
List<IndexName> disableIndexNameObjectList = removeDisabledCustomIndexesFromList(indexDefParentNode, reverseSortedIndexNameList);
// for disabled indexes, we check if they exist in read-only repo, it not anymore, do full deletion then, otherwise, no action needed
+ // Also skip deletion if ignoreIsIndexActiveCheck is true (which would be from ElasticPurgeOldIndexVersion) - this would be for ES indexes where we don't have any way to determine if the disabled ES index is
Review Comment:
could you better describe this part?
`this would be for ES indexes where we don't have any way to determine if the disabled ES index is`
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexConstants.java:
##########
@@ -27,6 +27,11 @@ public interface IndexConstants {
String TYPE_PROPERTY_NAME = "type";
+ /**
+ * Property to hold the value for original index implementation in case the index type is set to disabled.
+ */
+ String ORIGINAL_TYPE_PROPERTY_NAME = "orig_type";
Review Comment:
two questions:
* should we use a hidden property here prefixing it with `:`?
* I would use the camel case notation to be consistent with the other constants (eg: `originalType` or `origType`)
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/indexversion/IndexVersionOperation.java:
##########
@@ -90,25 +93,28 @@ public static List<IndexVersionOperation> generateIndexVersionOperationList(Node
* @return This method returns an IndexVersionOperation list i.e indexNameObjectList marked with operations
*/
public static List<IndexVersionOperation> generateIndexVersionOperationList(NodeState rootNode, String parentPath,
- List<IndexName> indexNameObjectList, long purgeThresholdMillis, boolean shouldPurgeBaseIndex) {
+ List<IndexName> indexNameObjectList, long purgeThresholdMillis, boolean shouldPurgeBaseIndex, boolean ignoreIsIndexActiveCheck) {
NodeState indexDefParentNode = NodeStateUtils.getNode(rootNode, parentPath);
List<IndexName> reverseSortedIndexNameList = getReverseSortedIndexNameList(indexNameObjectList);
List<IndexVersionOperation> indexVersionOperationList = new LinkedList<>();
List<IndexName> disableIndexNameObjectList = removeDisabledCustomIndexesFromList(indexDefParentNode, reverseSortedIndexNameList);
// for disabled indexes, we check if they exist in read-only repo, it not anymore, do full deletion then, otherwise, no action needed
+ // Also skip deletion if ignoreIsIndexActiveCheck is true (which would be from ElasticPurgeOldIndexVersion) - this would be for ES indexes where we don't have any way to determine if the disabled ES index is
+ // So we skip deleting the disabled index here so that we don't delete a disabled base index by mistake.
+ // Summary - Delete disabled index if ignoreIsIndexActiveCheck = false (which would be when called from LucenePurgeOldIndexVersion) and hidden oak mount doesn't exists.
Review Comment:
typo
```suggestion
// Summary - Delete disabled index if ignoreIsIndexActiveCheck = false (which would be when called from LucenePurgeOldIndexVersion) and hidden oak mount doesn't exist.
```
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/indexversion/PurgeOldIndexVersion.java:
##########
@@ -166,13 +175,39 @@ private Iterable<String> getRepositoryIndexPaths(NodeStore store) throws CommitF
* @param commandlineIndexPaths indexpaths provided by user
* @return returns set of indexpaths which are to be considered for purging
*/
- private Set<String> filterIndexPaths(Iterable<String> repositoryIndexPaths, List<String> commandlineIndexPaths) {
+ private Set<String> filterIndexPaths(NodeStore store, Iterable<String> repositoryIndexPaths, List<String> commandlineIndexPaths) {
Set<String> filteredIndexPaths = new HashSet<>();
for (String commandlineIndexPath : commandlineIndexPaths) {
for (String repositoryIndexPath : repositoryIndexPaths) {
if (PurgeOldVersionUtils.isIndexChildNode(commandlineIndexPath, repositoryIndexPath)
|| PurgeOldVersionUtils.isBaseIndexEqual(commandlineIndexPath, repositoryIndexPath)) {
- filteredIndexPaths.add(repositoryIndexPath);
+ NodeState root = store.getRoot();
+ NodeBuilder rootBuilder = root.builder();
Review Comment:
can we move these two statements outside the nested fors
##########
oak-run-elastic/src/test/java/org/apache/jackrabbit/oak/index/ElasticRepositoryFixture.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.index;
+
+import org.apache.jackrabbit.oak.Oak;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticConnection;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexTracker;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticMetricHandler;
+import org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.elastic.query.ElasticIndexProvider;
+import org.apache.jackrabbit.oak.plugins.index.search.ExtractedTextCache;
+import org.apache.jackrabbit.oak.spi.commit.Observer;
Review Comment:
unused
```suggestion
```
##########
oak-run-elastic/src/test/java/org/apache/jackrabbit/oak/index/ElasticRepositoryFixture.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.index;
+
+import org.apache.jackrabbit.oak.Oak;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticConnection;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexTracker;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticMetricHandler;
+import org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.elastic.query.ElasticIndexProvider;
+import org.apache.jackrabbit.oak.plugins.index.search.ExtractedTextCache;
+import org.apache.jackrabbit.oak.spi.commit.Observer;
+import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.state.NodeStore;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+
+import java.io.File;
+import java.io.IOException;
+
+public class ElasticRepositoryFixture extends IndexRepositoryFixture {
+
+ private final ElasticConnection connection;
+
+ public ElasticRepositoryFixture(File dir, ElasticConnection connection) {
+ super(dir);
+ this.connection = connection;
+ }
+
+ public ElasticRepositoryFixture(File dir, NodeStore nodeStore, ElasticConnection connection) {
+ super(dir, nodeStore);
+ this.connection = connection;
+ }
Review Comment:
never invoked, can we remove it?
##########
oak-run-elastic/src/main/java/org/apache/jackrabbit/oak/indexversion/ElasticPurgeOldIndexVersion.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.indexversion;
+
+import co.elastic.clients.elasticsearch.indices.DeleteIndexResponse;
+import co.elastic.clients.elasticsearch.indices.ElasticsearchIndicesClient;
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticConnection;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexDefinition;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexNameHelper;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+
+import static org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexDefinition.TYPE_ELASTICSEARCH;
+
+public class ElasticPurgeOldIndexVersion extends PurgeOldIndexVersion {
+ private static final Logger LOG = LoggerFactory.getLogger(ElasticPurgeOldIndexVersion.class);
+
+ private final String indexPrefix;
+ private final String scheme;
+ private final String host;
+ private final int port;
+ private final String apiKeyId;
+ private final String apiSecretId;
+ private long SEED_VALUE;
+
+ public ElasticPurgeOldIndexVersion(String indexPrefix, String scheme, String host, int port, String apiKeyId, String apiSecretId) {
+ super();
+ this.indexPrefix = indexPrefix;
+ this.scheme = scheme;
+ this.host = host;
+ this.port = port;
+ this.apiKeyId = apiKeyId;
+ this.apiSecretId = apiSecretId;
+ }
+
+ @Override
+ protected String getIndexType() {
+ return TYPE_ELASTICSEARCH;
+ }
+
+ @Override
+ protected void postDeleteOp(String idxPath) {
+ final ElasticConnection.Builder.BuildStep buildStep = ElasticConnection.newBuilder()
+ .withIndexPrefix(indexPrefix)
+ .withConnectionParameters(
+ scheme,
+ host,
+ port
+ );
+ final ElasticConnection coordinate;
+ if (apiKeyId != null && apiSecretId != null) {
+ coordinate = buildStep.withApiKeys(apiKeyId, apiSecretId).build();
+ } else {
+ coordinate = buildStep.build();
+ }
+
+ closer.register(coordinate);
+
+ String remoteIndexName = ElasticIndexNameHelper.getRemoteIndexName(indexPrefix, idxPath, SEED_VALUE);
+
+ ElasticsearchIndicesClient client = coordinate.getClient().indices();
+ DeleteIndexResponse deleteIndexResponse = null;
Review Comment:
```suggestion
DeleteIndexResponse deleteIndexResponse;
```
##########
oak-run-elastic/src/test/java/org/apache/jackrabbit/oak/index/ElasticRepositoryFixture.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.index;
+
+import org.apache.jackrabbit.oak.Oak;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticConnection;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexTracker;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticMetricHandler;
+import org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.elastic.query.ElasticIndexProvider;
+import org.apache.jackrabbit.oak.plugins.index.search.ExtractedTextCache;
+import org.apache.jackrabbit.oak.spi.commit.Observer;
+import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.state.NodeStore;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+
+import java.io.File;
+import java.io.IOException;
+
+public class ElasticRepositoryFixture extends IndexRepositoryFixture {
+
+ private final ElasticConnection connection;
+
+ public ElasticRepositoryFixture(File dir, ElasticConnection connection) {
+ super(dir);
+ this.connection = connection;
+ }
+
+ public ElasticRepositoryFixture(File dir, NodeStore nodeStore, ElasticConnection connection) {
+ super(dir, nodeStore);
+ this.connection = connection;
+ }
+
+ @Override
+ protected void configureIndexProvider(Oak oak) throws IOException {
Review Comment:
throws can be removed
```suggestion
protected void configureIndexProvider(Oak oak) {
```
##########
oak-run-elastic/src/test/java/org/apache/jackrabbit/oak/index/ElasticRepositoryFixture.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.index;
+
+import org.apache.jackrabbit.oak.Oak;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticConnection;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexTracker;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticMetricHandler;
+import org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.elastic.query.ElasticIndexProvider;
+import org.apache.jackrabbit.oak.plugins.index.search.ExtractedTextCache;
+import org.apache.jackrabbit.oak.spi.commit.Observer;
+import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.state.NodeStore;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+
+import java.io.File;
+import java.io.IOException;
+
+public class ElasticRepositoryFixture extends IndexRepositoryFixture {
+
+ private final ElasticConnection connection;
+
+ public ElasticRepositoryFixture(File dir, ElasticConnection connection) {
+ super(dir);
+ this.connection = connection;
+ }
+
+ public ElasticRepositoryFixture(File dir, NodeStore nodeStore, ElasticConnection connection) {
+ super(dir, nodeStore);
+ this.connection = connection;
+ }
+
+ @Override
+ protected void configureIndexProvider(Oak oak) throws IOException {
+ ElasticIndexTracker tracker = new ElasticIndexTracker(connection, new ElasticMetricHandler(StatisticsProvider.NOOP));
+
+ ElasticIndexEditorProvider ep = new ElasticIndexEditorProvider(tracker, connection,new ExtractedTextCache(0,0));
+ ElasticIndexProvider provider = new ElasticIndexProvider(tracker);
+ oak.with((QueryIndexProvider) provider)
Review Comment:
we should not need to cast it
```suggestion
oak.with(provider)
```
##########
oak-run/src/test/java/org/apache/jackrabbit/oak/index/LuceneRepositoryFixture.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.jackrabbit.oak.index;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.jackrabbit.oak.Oak;
+import org.apache.jackrabbit.oak.plugins.index.lucene.LuceneIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.lucene.LuceneIndexProvider;
+import org.apache.jackrabbit.oak.spi.commit.Observer;
+import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.state.NodeStore;
+
+public class LuceneRepositoryFixture extends IndexRepositoryFixture {
+
+ public LuceneRepositoryFixture(File dir) {
+ super(dir);
+ }
+
+ public LuceneRepositoryFixture(File dir, NodeStore nodeStore) {
+ super(dir, nodeStore);
+ }
+
+ @Override
+ protected void configureIndexProvider(Oak oak) throws IOException {
Review Comment:
throws can be removed
```suggestion
protected void configureIndexProvider(Oak oak) {
```
--
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.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org