[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #562: OAK-9760 Oak run index purge command active index check is in correct

2022-05-10 Thread GitBox 


thomasmueller commented on code in PR #562:
URL: https://github.com/apache/jackrabbit-oak/pull/562#discussion_r868920525


##
oak-run/src/main/java/org/apache/jackrabbit/oak/indexversion/IndexVersionOperation.java:
##
@@ -86,42 +92,69 @@ public static List 
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));
-
-// 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);
+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);

Review Comment:
   Not sure about the name "isActiveIndexLongEnough". Indexes are not "long" or 
"short". But indexes can be active for a long time / short time.
   
   What about one of the following:
   * isIndexActiveLongEnough (is the index active long enough?), or
   * isActiveIndexOldEnough (is the active index old enough?)
   
   



-- 
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

[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #562: OAK-9760 Oak run index purge command active index check is in correct

2022-05-06 Thread GitBox 


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 
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 
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