You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "nfsantos (via GitHub)" <gi...@apache.org> on 2023/08/30 07:35:00 UTC
[GitHub] [jackrabbit-oak] nfsantos commented on a diff in pull request #1066: OAK-10399 Automatically pick a merged index over multiple levels
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org