fabriziofortino commented on a change in pull request #370:
URL: https://github.com/apache/jackrabbit-oak/pull/370#discussion_r707177275



##########
File path: 
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexInfoProvider.java
##########
@@ -140,6 +139,24 @@ private static void computeLastUpdatedTime(NodeState 
idxState, LuceneIndexInfo i
         }
     }
 
+    private static void checkIfHiddenNodesExists(NodeState idxState, 
LuceneIndexInfo info) {
+        // Check for hidden oak libs mount node that has indexed content for 
read only repo in composite store
+        info.hasHiddenOakLibsMount = false;
+        for(String c : idxState.getChildNodeNames()) {
+            if (c.startsWith(IndexDefinition.HIDDEN_OAK_MOUNT_PREFIX)) {
+                info.hasHiddenOakLibsMount = true;
+            }
+        }
+
+        // Now check for hidden property index node :property-index - present 
in case of hybrid indexes
+        info.hasPropertyIndexNode = false;
+        for(String c : idxState.getChildNodeNames()) {
+            if (c.equals(IndexDefinition.PROPERTY_INDEX)) {
+                info.hasPropertyIndexNode = true;

Review comment:
       `break` ?

##########
File path: 
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexInfoProvider.java
##########
@@ -140,6 +139,24 @@ private static void computeLastUpdatedTime(NodeState 
idxState, LuceneIndexInfo i
         }
     }
 
+    private static void checkIfHiddenNodesExists(NodeState idxState, 
LuceneIndexInfo info) {
+        // Check for hidden oak libs mount node that has indexed content for 
read only repo in composite store
+        info.hasHiddenOakLibsMount = false;
+        for(String c : idxState.getChildNodeNames()) {
+            if (c.startsWith(IndexDefinition.HIDDEN_OAK_MOUNT_PREFIX)) {
+                info.hasHiddenOakLibsMount = true;

Review comment:
       `break`?

##########
File path: 
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexMBeanImpl.java
##########
@@ -122,13 +133,13 @@ public TabularData getIndexStats() throws IOException {
             TabularType tt = new 
TabularType(LuceneIndexMBeanImpl.class.getName(),
                     "Lucene Index Stats", IndexStats.TYPE, new 
String[]{"path"});
             tds = new TabularDataSupport(tt);
-            Set<String> indexes = indexTracker.getIndexNodePaths();
-            for (String path : indexes) {
+            // Use indexPathService to get list of all the lucene indexes.

Review comment:
       If I get it right, this is needed to show all the indexes while the 
`IndexTracker` returns the open ones only. Can we explain this in the comment?

##########
File path: 
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexInfoProvider.java
##########
@@ -140,6 +139,24 @@ private static void computeLastUpdatedTime(NodeState 
idxState, LuceneIndexInfo i
         }
     }
 
+    private static void checkIfHiddenNodesExists(NodeState idxState, 
LuceneIndexInfo info) {
+        // Check for hidden oak libs mount node that has indexed content for 
read only repo in composite store
+        info.hasHiddenOakLibsMount = false;
+        for(String c : idxState.getChildNodeNames()) {
+            if (c.startsWith(IndexDefinition.HIDDEN_OAK_MOUNT_PREFIX)) {
+                info.hasHiddenOakLibsMount = true;
+            }
+        }
+
+        // Now check for hidden property index node :property-index - present 
in case of hybrid indexes
+        info.hasPropertyIndexNode = false;
+        for(String c : idxState.getChildNodeNames()) {
+            if (c.equals(IndexDefinition.PROPERTY_INDEX)) {
+                info.hasPropertyIndexNode = true;
+            }
+        }
+    }

Review comment:
       Apart from terminating the conditions with a break, I think this logic 
can be simplified with a single for-loop.

##########
File path: 
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexMBeanImpl.java
##########
@@ -269,6 +280,10 @@ public boolean isFailing() {
         return getFieldTermPrefixInfo(indexPath, field, Integer.MAX_VALUE, 
term);
     }
 
+    private void bindIndexInfoProviders(IndexInfoServiceImpl indexInfoService) 
{

Review comment:
       this method is called only in the constructor. To improve the 
readability, I would move this logic in the constructor so the reader does not 
have to jump somewhere else to see what it does.

##########
File path: 
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexMBeanImpl.java
##########
@@ -675,7 +690,10 @@ private static Query newDepthQuery(int depth) {
                 "numDeletedDocs",
                 "nrtIndexSize",
                 "nrtIndexSizeStr",
-                "nrtNumDocs"
+                "nrtNumDocs",
+                "lastUpdatedTimeStamp",

Review comment:
       Timestamp can be written with one or two words. Usually, in software, 
the single word version is used.

##########
File path: 
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexInfoProvider.java
##########
@@ -140,6 +139,24 @@ private static void computeLastUpdatedTime(NodeState 
idxState, LuceneIndexInfo i
         }
     }
 
+    private static void checkIfHiddenNodesExists(NodeState idxState, 
LuceneIndexInfo info) {
+        // Check for hidden oak libs mount node that has indexed content for 
read only repo in composite store
+        info.hasHiddenOakLibsMount = false;
+        for(String c : idxState.getChildNodeNames()) {
+            if (c.startsWith(IndexDefinition.HIDDEN_OAK_MOUNT_PREFIX)) {

Review comment:
       I am not sure about this condition here. Instead of `startsWith` 
shouldn't we check for a node with name `:oak:mount-libs-index-data`? cc 
@thomasmueller 




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


Reply via email to