nfsantos commented on code in PR #1066:
URL: https://github.com/apache/jackrabbit-oak/pull/1066#discussion_r1309808775
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/IndexName.java:
##########
@@ -231,26 +231,37 @@ public String nextCustomizedName() {
}
public static boolean isIndexActive(String indexPath, NodeState rootState)
{
+ return isIndexActive(indexPath, rootState, 0);
+ }
+
+ private static boolean isIndexActive(String indexPath, NodeState
rootState, int recursionDepth) {
// An index is active if it has a hidden child node that starts with
":oak:mount-",
// OR if it is an active merged index
- try {
- NodeState indexNode = rootState;
- for (String e : PathUtils.elements(indexPath)) {
- indexNode = indexNode.getChildNode(e);
+ if (recursionDepth > 50) {
+ LOG.warn("Fail to check index activeness for {} due to high
recursion depth", indexPath);
+ return true;
+ }
+ NodeState indexNode = rootState;
+ for (String e : PathUtils.elements(indexPath)) {
+ indexNode = indexNode.getChildNode(e);
+ }
Review Comment:
This logic looks strange to me, it took me a while to understand it. It's
trying to get the child node of the last element on the indexPath, right? Can
you re-write this in a way that it is more clear? Something that reads more
like `PathUtils.elements(indexPath).last().getChildNode(e)`?
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/IndexName.java:
##########
@@ -231,26 +231,37 @@ public String nextCustomizedName() {
}
public static boolean isIndexActive(String indexPath, NodeState rootState)
{
+ return isIndexActive(indexPath, rootState, 0);
+ }
+
+ private static boolean isIndexActive(String indexPath, NodeState
rootState, int recursionDepth) {
// An index is active if it has a hidden child node that starts with
":oak:mount-",
// OR if it is an active merged index
- try {
- NodeState indexNode = rootState;
- for (String e : PathUtils.elements(indexPath)) {
- indexNode = indexNode.getChildNode(e);
+ if (recursionDepth > 50) {
+ LOG.warn("Fail to check index activeness for {} due to high
recursion depth", indexPath);
Review Comment:
Log the recursion depth. And could this happen because of cycles in the
index definition? We could explain in the log messages what are the possible
causes and fixes for this error.
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/IndexName.java:
##########
@@ -231,26 +231,37 @@ public String nextCustomizedName() {
}
public static boolean isIndexActive(String indexPath, NodeState rootState)
{
+ return isIndexActive(indexPath, rootState, 0);
+ }
+
+ private static boolean isIndexActive(String indexPath, NodeState
rootState, int recursionDepth) {
// An index is active if it has a hidden child node that starts with
":oak:mount-",
// OR if it is an active merged index
- try {
- NodeState indexNode = rootState;
- for (String e : PathUtils.elements(indexPath)) {
- indexNode = indexNode.getChildNode(e);
+ if (recursionDepth > 50) {
Review Comment:
Maybe the maximum recursion depth should be exposed as an internal
configuration property. 50 seems reasonable but maybe in the future we may want
to increase it. In the very least, this constant should be defined as a static
field in the class, so it has a clear name.
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/IndexName.java:
##########
@@ -231,26 +231,37 @@ public String nextCustomizedName() {
}
public static boolean isIndexActive(String indexPath, NodeState rootState)
{
+ return isIndexActive(indexPath, rootState, 0);
+ }
+
+ private static boolean isIndexActive(String indexPath, NodeState
rootState, int recursionDepth) {
// An index is active if it has a hidden child node that starts with
":oak:mount-",
// OR if it is an active merged index
- try {
- NodeState indexNode = rootState;
- for (String e : PathUtils.elements(indexPath)) {
- indexNode = indexNode.getChildNode(e);
+ if (recursionDepth > 50) {
+ LOG.warn("Fail to check index activeness for {} due to high
recursion depth", indexPath);
+ return true;
+ }
+ NodeState indexNode = rootState;
+ for (String e : PathUtils.elements(indexPath)) {
+ indexNode = indexNode.getChildNode(e);
+ }
+ for (String c : indexNode.getChildNodeNames()) {
+ if (c.startsWith(":oak:mount-")) {
+ return true;
}
- for (String c : indexNode.getChildNodeNames()) {
- if (c.startsWith(":oak:mount-")) {
- return true;
- }
+ }
+ if (recursionDepth >= 2) {
Review Comment:
Shouldn't it >= 1? The recursion starts at 0, so at level 1 we can already
have an OOTB index, no? Is there a test for this case? That is, for an index
`lucene-7-custom-1` with `merges=["lucene-7", "lucene-6-custom2"]`. And are
there tests for other cases? I don't see them in this PR or in the
IndexNameTest class.
--
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]
