thomasmueller commented on code in PR #562:
URL: https://github.com/apache/jackrabbit-oak/pull/562#discussion_r866548521
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/indexversion/IndexVersionOperation.java:
##########
@@ -86,42 +92,65 @@ public static List<IndexVersionOperation>
generateIndexVersionOperationList(Node
}
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));
+ IndexName activeIndexNameObject =
getActiveIndex(reverseSortedIndexNameList, parentPath, rootNode);
+ if (activeIndexNameObject == null) {
+ LOG.warn("Cannot find any active index from the list: {}",
reverseSortedIndexNameList);
+ } else {
+ 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
Review Comment:
I would make this comment a bit clearer:
```
// the reverseSortedIndexNameList will now contain the remaining indexes;
// the active index was removed from that list
```
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/indexversion/IndexVersionOperation.java:
##########
@@ -86,42 +92,65 @@ public static List<IndexVersionOperation>
generateIndexVersionOperationList(Node
}
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));
+ IndexName activeIndexNameObject =
getActiveIndex(reverseSortedIndexNameList, parentPath, rootNode);
+ if (activeIndexNameObject == null) {
+ LOG.warn("Cannot find any active index from the list: {}",
reverseSortedIndexNameList);
+ } else {
+ 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 {
- // 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);
+ 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 {
-
indexVersionOperation.setOperation(Operation.DELETE);
+ } else if (indexNameObject.getProductVersion() <=
activeProductVersion ) {
+ // 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);
+ }
}
+ // if the index product version is larger than active
index, leave it as is
+ // for instance: if there is active damAssetLucene-7,
the inactive damAssetLucene-8 will be leave there as is
Review Comment:
Shouldn't we add an "else" case where, where we also log this case (info
level), and set the operation? (Operation.NOOP). I know, Operation.NOOP is the
default (and that's good), so setting this to NOOP is a no-op :-) But when
reading this code here, that's not obvious.
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/indexversion/IndexVersionOperation.java:
##########
@@ -86,42 +92,65 @@ public static List<IndexVersionOperation>
generateIndexVersionOperationList(Node
}
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));
+ IndexName activeIndexNameObject =
getActiveIndex(reverseSortedIndexNameList, parentPath, rootNode);
+ if (activeIndexNameObject == null) {
+ LOG.warn("Cannot find any active index from the list: {}",
reverseSortedIndexNameList);
Review Comment:
I think it's fine to log a warning currently... I just wonder, how would we
remove an OOTB index from AEM. The first such case is the generic Lucene index
(/oak:index/lucene). We can remove all versions of that index in
GraniteContent. So there would never be this case that he have indexes, but
none is active (the old state is: active; the new state is: removed).
--
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-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
