You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "thomasmueller (via GitHub)" <gi...@apache.org> on 2023/08/14 15:22:22 UTC
[GitHub] [jackrabbit-oak] thomasmueller opened a new pull request, #1066: OAK-10399 Automatically pick a merged index over multiple levels
thomasmueller opened a new pull request, #1066:
URL: https://github.com/apache/jackrabbit-oak/pull/1066
(no comment)
--
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
[GitHub] [jackrabbit-oak] thomasmueller merged pull request #1066: OAK-10399 Automatically pick a merged index over multiple levels
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller merged PR #1066:
URL: https://github.com/apache/jackrabbit-oak/pull/1066
--
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
[GitHub] [jackrabbit-oak] nfsantos commented on a diff in pull request #1066: OAK-10399 Automatically pick a merged index over multiple levels
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
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
[GitHub] [jackrabbit-oak] sonarcloud[bot] commented on pull request #1066: OAK-10399 Automatically pick a merged index over multiple levels
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1066:
URL: https://github.com/apache/jackrabbit-oak/pull/1066#issuecomment-1695757689
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&metric=new_coverage&view=list) [88.2% Coverage](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&metric=new_duplicated_lines_density&view=list)
 The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more [here](https://docs.sonarcloud.io/appendices/scanner-environment/)
--
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
[GitHub] [jackrabbit-oak] sonarcloud[bot] commented on pull request #1066: OAK-10399 Automatically pick a merged index over multiple levels
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1066:
URL: https://github.com/apache/jackrabbit-oak/pull/1066#issuecomment-1698886255
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&metric=new_coverage&view=list) [88.9% Coverage](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1066&metric=new_duplicated_lines_density&view=list)
 The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more [here](https://docs.sonarcloud.io/appendices/scanner-environment/)
--
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
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #1066: OAK-10399 Automatically pick a merged index over multiple levels
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1066:
URL: https://github.com/apache/jackrabbit-oak/pull/1066#discussion_r1309908376
##########
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:
Logging the recursion depths seems only needed if we make it configurable. But OK, it doesn't hurt.
##########
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:
I'm not quite sure what the added indirection would help in this case: we only use the setting once. But OK, I'll add a constant.
##########
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:
Level 1 (the direct children) will still need to check.
> Is there a test for this case?
Yes: CustomizedIndexTest and IndexNameTest are in the patch. The test coverage of the changes is 100% as far as I see.
##########
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 is a _loop_ over all the entries in the path, to go down to the respective node. This code is not changed.
--
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
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #1066: OAK-10399 Automatically pick a merged index over multiple levels
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1066:
URL: https://github.com/apache/jackrabbit-oak/pull/1066#discussion_r1309908376
##########
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:
Logging the recursion depths seems only useful if we make it configurable. But OK, it doesn't hurt.
--
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