nfsantos commented on code in PR #1180:
URL: https://github.com/apache/jackrabbit-oak/pull/1180#discussion_r1372641880
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexInfoServiceImpl.java:
##########
@@ -62,11 +63,21 @@ public IndexInfoServiceImpl(NodeStore nodeStore,
IndexPathService indexPathServi
@Override
public Iterable<IndexInfo> getAllIndexInfo() {
+ HashSet<String> allIndexes = new HashSet<>();
+ indexPathService.getIndexPaths().forEach(allIndexes::add);
+ HashSet<String> activeIndexes = new HashSet<>();
+ if (indexPathService.getMountInfoProvider().hasNonDefaultMounts()) {
+ IndexName.filterReplacedIndexes(allIndexes, nodeStore.getRoot(),
true).forEach(activeIndexes::add);
Review Comment:
```suggestion
activeIndexes.addAll(IndexName.filterReplacedIndexes(allIndexes,
nodeStore.getRoot(), true));
```
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/composite/blueGreen/CustomizedIndexTest.java:
##########
@@ -100,6 +112,8 @@ private void compositeLibs2() throws Exception {
"/jcr:root//*[@foo] order by @jcr:path",
"test-2",
"[/content/test, /libs/test2]");
+ assertEquals("[/oak:index/lucene, /oak:index/test-2]",
Review Comment:
There is no test case with no active indexes. Although this probably should
not happen in practice, it may be good to have that test case. And also a test
case with a single active index.
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/composite/blueGreen/CustomizedIndexTest.java:
##########
@@ -211,4 +233,25 @@ private void createFolders() throws IOException {
indexDir = tempDir.newFolder("index");
}
+ private static String getActiveLuceneIndexes(Persistence p) {
+ return getActiveLuceneIndexes(p.getCompositeNodestore(),
p.getMountInfoProvider());
+ }
+
+ private static String getActiveLuceneIndexes(NodeStore ns,
MountInfoProvider m) {
+ ArrayList<String> list = new ArrayList<>();
+ IndexInfoServiceImpl indexService = new IndexInfoServiceImpl(ns,
+ new IndexPathServiceImpl(ns, m));
+ for (IndexInfo info : indexService.getAllIndexInfo()) {
+ if (!LuceneIndexConstants.TYPE_LUCENE.equals(info.getType())) {
+ continue;
+ }
+ if (!info.isActive()) {
+ continue;
+ }
+ list.add(info.getIndexPath());
+ }
+ Collections.sort(list);
+ return list.toString();
+ }
Review Comment:
I find the name of this method confusing. From the name, without looking at
the return type, I expected that it returns a list of the active indexes. But
in fact, this converts to String description, so it should have a name like
toString().
Why are you doing the test assertions on a string representation of the list
of the indexes instead of using Java lists or Java sets? I think using lists or
sets is less error prone and more clear, because conceptually that is what we
are comparing, not strings.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexInfoServiceImpl.java:
##########
@@ -62,11 +63,21 @@ public IndexInfoServiceImpl(NodeStore nodeStore,
IndexPathService indexPathServi
@Override
public Iterable<IndexInfo> getAllIndexInfo() {
+ HashSet<String> allIndexes = new HashSet<>();
+ indexPathService.getIndexPaths().forEach(allIndexes::add);
+ HashSet<String> activeIndexes = new HashSet<>();
+ if (indexPathService.getMountInfoProvider().hasNonDefaultMounts()) {
+ IndexName.filterReplacedIndexes(allIndexes, nodeStore.getRoot(),
true).forEach(activeIndexes::add);
+ } else {
+ activeIndexes.addAll(allIndexes);
+ }
return
Iterables.filter(Iterables.transform(indexPathService.getIndexPaths(), new
Function<String, IndexInfo>() {
@Override
public IndexInfo apply(String indexPath) {
try {
- return getInfo(indexPath);
+ IndexInfo info = getInfo(indexPath);
+ info.setActive(activeIndexes.contains(indexPath));
+ return info;
Review Comment:
Is it possible for getInfo to return null? the method can return null in
some conditions, could it happen in this particular context? If so, guard
against that case when calling `setActive`.
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/IndexName.java:
##########
@@ -33,16 +33,8 @@
import org.slf4j.LoggerFactory;
/**
- * An index name, which possibly contains two version numbers: the product
- * version number, and the customer version number.
- *
- * The format of an index node name is:
- * - The name of the index,
- * - optionally a dash ('-') and the product version number,
- * - optionally "-custom-" and the customer version number.
- *
- * If the node name doesn't contain version numbers / dashes, then version 0 is
- * assumed (for both the product version number and customer version number).
+ * @deprecated
+ * Use oak-core org.apache.jackrabbit.oak.plugins.index.IndexName instead.
Review Comment:
Is it enough to have `@deprecated` in the comments, without having the
`@Deprecated` annotation on the 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]
