fabriziofortino commented on code in PR #671:
URL: https://github.com/apache/jackrabbit-oak/pull/671#discussion_r950028769
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticResponseHandler.java:
##########
@@ -41,14 +43,22 @@ public class ElasticResponseHandler {
private final PlanResult planResult;
private final Filter filter;
+ private final Set<String> seenPaths = Sets.newHashSet();
Review Comment:
I would just `new HashSet<>()` here. The google common method does not add
anything (it's just a wrapper). We are trying to reduce, and possibly avoid,
the use of guava in this module.
##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatisticsTest.java:
##########
@@ -70,7 +70,7 @@ public void defaultIndexStatistics() {
public void cachedStatistics() throws Exception {
MutableTicker ticker = new MutableTicker();
LoadingCache<ElasticIndexStatistics.StatsRequestDescriptor, Integer>
cache =
- ElasticIndexStatistics.setupCountCache(100, 10, 1, ticker);
+ ElasticIndexStatistics.setupCountCache(100, 10*60, 1*60,
ticker);
Review Comment:
```suggestion
ElasticIndexStatistics.setupCountCache(100, 10 * 60, 60,
ticker);
```
##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexPathRestrictionCommonTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.index.elastic;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.jackrabbit.oak.commons.junit.LogCustomizer;
+import org.apache.jackrabbit.oak.plugins.index.IndexPathRestrictionCommonTest;
+import org.apache.jackrabbit.oak.plugins.index.IndexUpdateProvider;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticIndexEditorProvider;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.query.ElasticIndexProvider;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.query.async.ElasticResultRowAsyncIterator;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.plugins.index.search.ExtractedTextCache;
+import org.apache.jackrabbit.oak.plugins.index.search.spi.query.FulltextIndex;
+import
org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.spi.commit.EditorHook;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+import org.junit.ClassRule;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.slf4j.event.Level;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+@RunWith(Parameterized.class)
+public class ElasticIndexPathRestrictionCommonTest extends
IndexPathRestrictionCommonTest {
+
+ @ClassRule
+ public static final ElasticConnectionRule elasticRule =
+ new
ElasticConnectionRule(ElasticTestUtils.ELASTIC_CONNECTION_STRING);
+
+ private ElasticConnection esConnection;
+ private ElasticIndexTracker indexTracker;
+
+
+ public ElasticIndexPathRestrictionCommonTest(boolean
evaluatePathRestrictionsInIndex) {
+ super(evaluatePathRestrictionsInIndex);
+ }
+
+ @Parameterized.Parameters(name = "evaluatePathRestrictionsInIndex = {0}")
+ public static Collection<Object[]> data() {
+ // For Elastic - evaluatePathRestrictions is effectively moot since we
always enable path restrictions by default.
+ // And always index ancestors in ElasticDocumentMaker#finalizeDoc
+ // So we test for only 1 combination here -
evaluatePathRestrictions=true
+ return Arrays.asList(new Object[][]{
+ doesIndexEvaluatePathRestrictions(true)
+ });
+ }
+
+ @Override
+ protected void postCommitHooks() {
+ indexTracker.update(root);
+ }
+
+ @Override
+ protected void setupHook() {
+ // Default refresh is 1 minute - so we need to lower that otherwise
test would need to wait at least 1 minute
+ // before it can get the estimated doc count from the remote ES index
+ System.setProperty("oak.elastic.statsRefreshSeconds", "10");
Review Comment:
can we use the test rules to manage system properties? see
`ElasticIndexWriterDisabledTest`
##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexPathRestrictionCommonTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.index.elastic;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.jackrabbit.oak.commons.junit.LogCustomizer;
+import org.apache.jackrabbit.oak.plugins.index.IndexPathRestrictionCommonTest;
+import org.apache.jackrabbit.oak.plugins.index.IndexUpdateProvider;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticIndexEditorProvider;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.query.ElasticIndexProvider;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.query.async.ElasticResultRowAsyncIterator;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.plugins.index.search.ExtractedTextCache;
+import org.apache.jackrabbit.oak.plugins.index.search.spi.query.FulltextIndex;
+import
org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.spi.commit.EditorHook;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+import org.junit.ClassRule;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.slf4j.event.Level;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+@RunWith(Parameterized.class)
+public class ElasticIndexPathRestrictionCommonTest extends
IndexPathRestrictionCommonTest {
+
+ @ClassRule
+ public static final ElasticConnectionRule elasticRule =
+ new
ElasticConnectionRule(ElasticTestUtils.ELASTIC_CONNECTION_STRING);
+
+ private ElasticConnection esConnection;
+ private ElasticIndexTracker indexTracker;
+
+
+ public ElasticIndexPathRestrictionCommonTest(boolean
evaluatePathRestrictionsInIndex) {
+ super(evaluatePathRestrictionsInIndex);
+ }
+
+ @Parameterized.Parameters(name = "evaluatePathRestrictionsInIndex = {0}")
+ public static Collection<Object[]> data() {
+ // For Elastic - evaluatePathRestrictions is effectively moot since we
always enable path restrictions by default.
+ // And always index ancestors in ElasticDocumentMaker#finalizeDoc
+ // So we test for only 1 combination here -
evaluatePathRestrictions=true
+ return Arrays.asList(new Object[][]{
+ doesIndexEvaluatePathRestrictions(true)
+ });
Review Comment:
```suggestion
return
Collections.singleton(doesIndexEvaluatePathRestrictions(true));
```
##########
oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexPathRestrictionCommonTest.java:
##########
@@ -0,0 +1,609 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.index;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import org.apache.jackrabbit.oak.InitialContentHelper;
+import org.apache.jackrabbit.oak.commons.junit.LogCustomizer;
+import org.apache.jackrabbit.oak.plugins.index.search.spi.query.FulltextIndex;
+import
org.apache.jackrabbit.oak.plugins.index.search.spi.query.FulltextIndexPlanner;
+import
org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyValues;
+import org.apache.jackrabbit.oak.query.NodeStateNodeTypeInfoProvider;
+import org.apache.jackrabbit.oak.query.QueryEngineSettings;
+import org.apache.jackrabbit.oak.query.ast.NodeTypeInfo;
+import org.apache.jackrabbit.oak.query.ast.NodeTypeInfoProvider;
+import org.apache.jackrabbit.oak.query.ast.Operator;
+import org.apache.jackrabbit.oak.query.ast.SelectorImpl;
+import org.apache.jackrabbit.oak.query.index.FilterImpl;
+import org.apache.jackrabbit.oak.spi.commit.CommitInfo;
+import org.apache.jackrabbit.oak.spi.commit.EditorHook;
+import org.apache.jackrabbit.oak.spi.query.Cursor;
+import org.apache.jackrabbit.oak.spi.query.Filter;
+import org.apache.jackrabbit.oak.spi.query.QueryIndex;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeState;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static com.google.common.collect.ImmutableSet.of;
+import static org.apache.jackrabbit.JcrConstants.NT_BASE;
+import static org.junit.Assert.assertEquals;
+
+@RunWith(Parameterized.class)
+public abstract class IndexPathRestrictionCommonTest {
+
+ protected EditorHook HOOK;
Review Comment:
lowercase
```suggestion
protected EditorHook hook;
```
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatistics.java:
##########
@@ -42,26 +42,26 @@
* Cache-based {@code IndexStatistics} implementation providing statistics for
Elasticsearch reducing
* network operations.
* <p>
- * By default, the cache can contain a max of 10000 entries, statistic values
expire after 10 minutes but are refreshed
- * in background when accessed after 1 minute. These values can be overwritten
with the following system properties:
+ * By default, the cache can contain a max of 10000 entries, statistic values
expire after 10 minutes (600 seconds) but are refreshed
+ * in background when accessed after 1 minute (60 seconds). These values can
be overwritten with the following system properties:
*
* <ul>
* <li>{@code oak.elastic.statsMaxSize}</li>
- * <li>{@code oak.elastic.statsExpireMin}</li>
- * <li>{@code oak.elastic.statsRefreshMin}</li>
+ * <li>{@code oak.elastic.statsExpireSeconds}</li>
+ * <li>{@code oak.elastic.statsRefreshSeconds}</li>
* </ul>
*/
public class ElasticIndexStatistics implements IndexStatistics {
private static final Long MAX_SIZE =
Long.getLong("oak.elastic.statsMaxSize", 10000);
- private static final Long EXPIRE_MIN =
Long.getLong("oak.elastic.statsExpireMin", 10);
- private static final Long REFRESH_MIN =
Long.getLong("oak.elastic.statsRefreshMin", 1);
+ private static final Long EXPIRE_SECONDS =
Long.getLong("oak.elastic.statsExpireSeconds", 10*60);
+ private static final Long REFRESH_SECONDS =
Long.getLong("oak.elastic.statsRefreshSeconds", 1*60);
Review Comment:
```suggestion
private static final Long EXPIRE_SECONDS =
Long.getLong("oak.elastic.statsExpireSeconds", 10 * 60);
private static final Long REFRESH_SECONDS =
Long.getLong("oak.elastic.statsRefreshSeconds", 60);
```
##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexPathRestrictionCommonTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.index.elastic;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.jackrabbit.oak.commons.junit.LogCustomizer;
+import org.apache.jackrabbit.oak.plugins.index.IndexPathRestrictionCommonTest;
+import org.apache.jackrabbit.oak.plugins.index.IndexUpdateProvider;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticIndexEditorProvider;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.query.ElasticIndexProvider;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.query.async.ElasticResultRowAsyncIterator;
+import
org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.plugins.index.search.ExtractedTextCache;
+import org.apache.jackrabbit.oak.plugins.index.search.spi.query.FulltextIndex;
+import
org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.spi.commit.EditorHook;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+import org.junit.ClassRule;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.slf4j.event.Level;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+@RunWith(Parameterized.class)
+public class ElasticIndexPathRestrictionCommonTest extends
IndexPathRestrictionCommonTest {
+
+ @ClassRule
+ public static final ElasticConnectionRule elasticRule =
+ new
ElasticConnectionRule(ElasticTestUtils.ELASTIC_CONNECTION_STRING);
+
+ private ElasticConnection esConnection;
Review Comment:
this is used in `setupHook()` and could be converted to a local variable
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexPathRestrictionCommonTest.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.index.lucene;
+
+import org.apache.jackrabbit.oak.commons.junit.LogCustomizer;
+import org.apache.jackrabbit.oak.plugins.index.IndexPathRestrictionCommonTest;
+import org.apache.jackrabbit.oak.plugins.index.IndexUpdateProvider;
+import
org.apache.jackrabbit.oak.plugins.index.lucene.util.LuceneIndexDefinitionBuilder;
+import
org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.spi.commit.EditorHook;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.slf4j.event.Level;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+@RunWith(Parameterized.class)
+public class LuceneIndexPathRestrictionCommonTest extends
IndexPathRestrictionCommonTest {
+
+ private IndexTracker tracker;
+
+ public LuceneIndexPathRestrictionCommonTest(boolean
evaluatePathRestrictions) {
+ super(evaluatePathRestrictions);
+ }
+
+ @Parameterized.Parameters(name = "evaluatePathRestrictionsInIndex = {0}")
+ public static Collection<Object[]> data() {
+ // For Lucene - evaluatePathRestrictions is used to enable/disable
indexing ancestor paths.
+ // So we test for both combinations here -
evaluatePathRestrictions=true and evaluatePathRestrictions=false
+ return Arrays.asList(new Object[][]{
+ doesIndexEvaluatePathRestrictions(true),
+ doesIndexEvaluatePathRestrictions(false)
+ });
Review Comment:
this can be simplified using
```suggestion
return Arrays.asList(doesIndexEvaluatePathRestrictions(true),
doesIndexEvaluatePathRestrictions(false));
```
--
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]
