thomasmueller commented on a change in pull request #525:
URL: https://github.com/apache/jackrabbit-oak/pull/525#discussion_r831864110
##########
File path:
oak-run/src/main/java/org/apache/jackrabbit/oak/indexversion/IndexVersionOperation.java
##########
@@ -70,58 +70,93 @@ public String toString() {
*/
public static List<IndexVersionOperation>
generateIndexVersionOperationList(NodeState indexDefParentNode,
List<IndexName> indexNameObjectList, long purgeThresholdMillis) {
- int activeProductVersion = -1;
List<IndexName> reverseSortedIndexNameList =
getReverseSortedIndexNameList(indexNameObjectList);
- removeDisabledCustomIndexesFromList(indexDefParentNode,
reverseSortedIndexNameList);
List<IndexVersionOperation> indexVersionOperationList = new
LinkedList<>();
- for (IndexName indexNameObject : reverseSortedIndexNameList) {
- NodeState indexNode = indexDefParentNode
-
.getChildNode(PathUtils.getName(indexNameObject.getNodeName()));
- /*
- All Indexes are by default NOOP until we get index which is active
for more than threshold limit
- */
- if (activeProductVersion == -1) {
- indexVersionOperationList.add(new
IndexVersionOperation(indexNameObject));
- if (indexNode.hasChildNode(IndexDefinition.STATUS_NODE)) {
- if (indexNode.getChildNode(IndexDefinition.STATUS_NODE)
-
.getProperty(IndexDefinition.REINDEX_COMPLETION_TIMESTAMP) != null) {
- String reindexCompletionTime = indexDefParentNode
-
.getChildNode(PathUtils.getName(indexNameObject.getNodeName()))
- .getChildNode(IndexDefinition.STATUS_NODE)
-
.getProperty(IndexDefinition.REINDEX_COMPLETION_TIMESTAMP).getValue(Type.DATE);
- long reindexCompletionTimeInMillis =
PurgeOldVersionUtils.getMillisFromString(reindexCompletionTime);
- long currentTimeInMillis = System.currentTimeMillis();
- if (currentTimeInMillis -
reindexCompletionTimeInMillis > purgeThresholdMillis) {
- activeProductVersion =
indexNameObject.getProductVersion();
- }
+
+ 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
+ for (IndexName indexNameObject : disableIndexNameObjectList) {
+ String indexName = indexNameObject.getNodeName();
+ NodeState indexNode =
indexDefParentNode.getChildNode(PathUtils.getName(indexName));
+ IndexVersionOperation indexVersionOperation = new
IndexVersionOperation(indexNameObject);
+ if (!isHiddenOakMountExists(indexNode)) {
+ indexVersionOperation.setOperation(Operation.DELETE);
+ }
+ indexVersionOperationList.add(indexVersionOperation);
+ }
+
+ if (!reverseSortedIndexNameList.isEmpty()) {
+ IndexName activeIndexNameObject =
reverseSortedIndexNameList.remove(0);
+ NodeState activeIndexNode =
indexDefParentNode.getChildNode(PathUtils.getName(activeIndexNameObject.getNodeName()));
+ boolean isActiveIndexLongEnough =
isIndexPurgeReady(activeIndexNameObject, activeIndexNode, purgeThresholdMillis);
+ int activeProductVersion =
activeIndexNameObject.getProductVersion();
+ indexVersionOperationList.add(new
IndexVersionOperation(activeIndexNameObject));
+
+ // for rest indexes except active index
+ for (IndexName indexNameObject : reverseSortedIndexNameList) {
+ String indexName = indexNameObject.getNodeName();
+ NodeState indexNode =
indexDefParentNode.getChildNode(PathUtils.getName(indexName));
+ IndexVersionOperation indexVersionOperation = new
IndexVersionOperation(indexNameObject);
+ // if active index not long enough, NOOP for all indexes
+ if (isActiveIndexLongEnough) {
+ if (indexNameObject.getProductVersion() ==
activeProductVersion && indexNameObject.getCustomerVersion() == 0) {
+
indexVersionOperation.setOperation(Operation.DELETE_HIDDEN_AND_DISABLE);
} else {
- LOG.warn(IndexDefinition.REINDEX_COMPLETION_TIMESTAMP
- + " property is not set for index " +
indexNameObject.getNodeName());
+ // the check hidden oak mount logic only works when
passing through the proper composite store
+ if (isHiddenOakMountExists(indexNode)) {
+ LOG.info("Found hidden oak mount node for: '{}',
disable it but no index definition deletion", indexName);
+
indexVersionOperation.setOperation(Operation.DELETE_HIDDEN_AND_DISABLE);
+ } else {
+
indexVersionOperation.setOperation(Operation.DELETE);
+ }
}
}
- } else {
- /*
- Once we found active index, we mark all its custom version with
DELETE
- and OOTB same product version index with DELETE_HIDDEN_AND_DISABLE
- */
- if (indexNameObject.getProductVersion() == activeProductVersion
- && indexNameObject.getCustomerVersion() == 0) {
- IndexVersionOperation indexVersionOperation = new
IndexVersionOperation(indexNameObject);
-
indexVersionOperation.setOperation(Operation.DELETE_HIDDEN_AND_DISABLE);
- indexVersionOperationList.add(indexVersionOperation);
- } else {
- IndexVersionOperation indexVersionOperation = new
IndexVersionOperation(indexNameObject);
- indexVersionOperation.setOperation(Operation.DELETE);
- indexVersionOperationList.add(indexVersionOperation);
- }
+ LOG.info("The operation for index '{}' will be: '{}'",
indexName, indexVersionOperation.getOperation());
+ indexVersionOperationList.add(indexVersionOperation);
}
}
if (!isValidIndexVersionOperationList(indexVersionOperationList)) {
+ LOG.info("Not valid version operation list: '{}', skip all",
indexNameObjectList);
indexVersionOperationList = Collections.emptyList();
}
return indexVersionOperationList;
}
+ // do index purge ready based on the active index's last reindexing time
is longer enough, we do this for prevent rollback
+ private static boolean isIndexPurgeReady(IndexName activeIndexName,
NodeState activeIndexNode, long purgeThresholdMillis) {
+ // the 1st index must be active
+ String indexName = activeIndexName.getNodeName();
+ if (activeIndexNode.hasChildNode(IndexDefinition.STATUS_NODE)) {
+ if (activeIndexNode.getChildNode(IndexDefinition.STATUS_NODE)
+ .getProperty(IndexDefinition.REINDEX_COMPLETION_TIMESTAMP)
!= null) {
+ String reindexCompletionTime =
activeIndexNode.getChildNode(IndexDefinition.STATUS_NODE)
+
.getProperty(IndexDefinition.REINDEX_COMPLETION_TIMESTAMP)
+ .getValue(Type.DATE);
+ long reindexCompletionTimeInMillis =
PurgeOldVersionUtils.getMillisFromString(reindexCompletionTime);
+ long currentTimeInMillis = System.currentTimeMillis();
+ if (currentTimeInMillis - reindexCompletionTimeInMillis >
purgeThresholdMillis) {
+ LOG.info("Found active index {} is longer enough",
indexName);
+ return true;
+ } else {
+ LOG.info("The last index time '{}' isn't longer enough
for: {}", reindexCompletionTime, indexName);
+ }
+ } else {
+ LOG.warn("{} property is not set for index {}",
IndexDefinition.REINDEX_COMPLETION_TIMESTAMP, indexName);
Review comment:
I think the logic is correct... but how can we set the value if it is
missing? Currently I'm not sure where we should set the property... Maybe we
could set it right here (I think we have write access), to the current date.
##########
File path:
oak-run/src/test/java/org/apache/jackrabbit/oak/indexversion/PurgeOldIndexVersionTest.java
##########
@@ -80,39 +85,63 @@ public void deleteOldIndexCompletely() throws Exception {
createCustomIndex(TEST_INDEX_PATH, 4, 0, false);
createCustomIndex(TEST_INDEX_PATH, 4, 1, false);
createCustomIndex(TEST_INDEX_PATH, 4, 2, false);
- fixture.getAsyncIndexUpdate("async").run();
- fixture.close();
- PurgeOldIndexVersionCommand command = new
PurgeOldIndexVersionCommand();
- File storeDir = fixture.getDir();
- String[] args = {
- storeDir.getAbsolutePath(),
- "--read-write",
- "--threshold=1"
- };
- command.execute(args);
- fixture = new RepositoryFixture(storeDir);
- fixture.close();
+ runIndexPurgeCommand(true, 1, "");
+
+ NodeState indexRootNode =
fixture.getNodeStore().getRoot().getChildNode("oak:index");
- Assert.assertFalse("Index:" + "fooIndex-2" + " deleted",
fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex-2").exists());
- Assert.assertFalse("Index:" + "fooIndex-2-custom-1" + " deleted",
fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex-2-custom-1").exists());
- Assert.assertFalse("Index:" + "fooIndex-3" + " deleted",
fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex-3").exists());
- Assert.assertFalse("Index:" + "fooIndex-3-custom-1" + " deleted",
fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex-3-custom-1").exists());
- Assert.assertFalse("Index:" + "fooIndex-3-custom-2" + " deleted",
fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex-3-custom-2").exists());
- Assert.assertFalse("Index:" + "fooIndex" + " deleted",
fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex").exists());
-
Assert.assertEquals(fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex-4").getProperty("type").getValue(Type.STRING),
"disabled");
-
Assert.assertFalse(isHiddenChildNodePresent(fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex-4")));
- Assert.assertFalse("Index:" + "fooIndex-4-custom-1" + " deleted",
fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex-4-custom-1").exists());
- Assert.assertTrue("Index:" + "fooIndex-4-custom-2" + " deleted",
fixture.getNodeStore().getRoot().getChildNode("oak:index").getChildNode("fooIndex-4-custom-2").exists());
+ Assert.assertFalse("Index:" + "fooIndex-2" + " deleted",
indexRootNode.getChildNode("fooIndex-2").exists());
+ Assert.assertFalse("Index:" + "fooIndex-2-custom-1" + " deleted",
indexRootNode.getChildNode("fooIndex-2-custom-1").exists());
+ Assert.assertFalse("Index:" + "fooIndex-3" + " deleted",
indexRootNode.getChildNode("fooIndex-3").exists());
+ Assert.assertFalse("Index:" + "fooIndex-3-custom-1" + " deleted",
indexRootNode.getChildNode("fooIndex-3-custom-1").exists());
+ Assert.assertFalse("Index:" + "fooIndex-3-custom-2" + " deleted",
indexRootNode.getChildNode("fooIndex-3-custom-2").exists());
+ Assert.assertFalse("Index:" + "fooIndex" + " deleted",
indexRootNode.getChildNode("fooIndex").exists());
+
Assert.assertEquals(indexRootNode.getChildNode("fooIndex-4").getProperty("type").getValue(Type.STRING),
"disabled");
Review comment:
Nit: I would swap the parameters (the literal "disabled" should come
first)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
