jayblanc commented on code in PR #729: URL: https://github.com/apache/unomi/pull/729#discussion_r2344223548
########## persistence-elasticsearch/conditions/pom.xml: ########## @@ -0,0 +1,198 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> + +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-persistence-elasticsearch</artifactId> + <version>3.0.0-SNAPSHOT</version> + </parent> + + <artifactId>unomi-persistence-elasticsearch-conditions</artifactId> + <name>Apache Unomi :: Persistence :: ElasticSearch :: Conditions</name> + <description>Conditions ElasticSearch persistence implementation for the Apache Unomi Context Server</description> + <packaging>bundle</packaging> + + <dependencyManagement> + <dependencies> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-bom</artifactId> + <version>${project.version}</version> + <type>pom</type> + <scope>import</scope> + </dependency> + </dependencies> + </dependencyManagement> + <dependencies> + <dependency> + <groupId>org.osgi</groupId> + <artifactId>osgi.core</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-api</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-common</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-persistence-spi</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.google.guava</groupId> + <artifactId>guava</artifactId> + <version>${guava.version}</version> Review Comment: Guava version is already setup in the BOM so version is not needed here. It is also included in the feature so it should be flagged as provided. Anyway, Guava is a huge dependency we tried to remove in other project/module. If another option is possible, it's better to remove guava. I don't see direct usage but maybe a library is using it... ########## persistence-elasticsearch/conditions/pom.xml: ########## @@ -0,0 +1,198 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> + +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-persistence-elasticsearch</artifactId> + <version>3.0.0-SNAPSHOT</version> + </parent> + + <artifactId>unomi-persistence-elasticsearch-conditions</artifactId> + <name>Apache Unomi :: Persistence :: ElasticSearch :: Conditions</name> + <description>Conditions ElasticSearch persistence implementation for the Apache Unomi Context Server</description> + <packaging>bundle</packaging> + + <dependencyManagement> + <dependencies> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-bom</artifactId> + <version>${project.version}</version> + <type>pom</type> + <scope>import</scope> + </dependency> + </dependencies> + </dependencyManagement> + <dependencies> + <dependency> + <groupId>org.osgi</groupId> + <artifactId>osgi.core</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-api</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-common</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-persistence-spi</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.google.guava</groupId> + <artifactId>guava</artifactId> + <version>${guava.version}</version> + </dependency> + + <dependency> + <groupId>com.fasterxml.jackson.core</groupId> + <artifactId>jackson-databind</artifactId> + </dependency> + + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-lang3</artifactId> + </dependency> + <dependency> + <groupId>commons-collections</groupId> + <artifactId>commons-collections</artifactId> + </dependency> + <dependency> + <groupId>commons-io</groupId> + <artifactId>commons-io</artifactId> + </dependency> + <dependency> + <groupId>co.elastic.clients</groupId> + <artifactId>elasticsearch-java</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.elasticsearch.client</groupId> + <artifactId>elasticsearch-rest-client</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-metrics</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + + <dependency> + <groupId>junit</groupId> + <artifactId>junit</artifactId> + <scope>test</scope> + </dependency> + <dependency> + <groupId>com.hazelcast</groupId> + <artifactId>hazelcast-all</artifactId> + <version>3.12.8</version> + <scope>provided</scope> + </dependency> + + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-scripting</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-persistence-elasticsearch-core</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + + <dependency> + <groupId>joda-time</groupId> + <artifactId>joda-time</artifactId> + <version>2.12.7</version> Review Comment: This is already present Unomi BOM and version in Unomi Parent pom.xml, version is not needed. Also dependencies are grouped with a logic : - unomi modules dependencies are on top - embedded dependencies are grouped after - provided dependencies then - test dependencies finally. It's better to move each dependency accordingly to keep an easier reading approach. ########## metrics/src/main/java/org/apache/unomi/metrics/commands/ViewCommand.java: ########## @@ -40,7 +40,7 @@ protected Object doExecute() throws Exception { // the caller values easier to read. DefaultPrettyPrinter defaultPrettyPrinter = new DefaultPrettyPrinter(); defaultPrettyPrinter = defaultPrettyPrinter.withArrayIndenter(DefaultIndenter.SYSTEM_LINEFEED_INSTANCE); - String jsonMetric = CustomObjectMapper.getObjectMapper().writer(defaultPrettyPrinter).writeValueAsString(metric); + String jsonMetric = new ObjectMapper().writer(defaultPrettyPrinter).writeValueAsString(metric); Review Comment: If CustomObjectMapper is no more used, maybe we could use another ObjectMapper reference avoiding building a new one on each call. ########## persistence-elasticsearch/core/pom.xml: ########## @@ -78,6 +80,14 @@ <scope>provided</scope> </dependency> + <dependency> + <groupId>co.elastic.clients</groupId> + <artifactId>elasticsearch-java</artifactId> + </dependency> + <dependency> + <groupId>org.elasticsearch.client</groupId> + <artifactId>elasticsearch-rest-client</artifactId> + </dependency> Review Comment: Maybe we can remove hazelcast from here too. ########## persistence-elasticsearch/conditions/pom.xml: ########## @@ -0,0 +1,198 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> + +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-persistence-elasticsearch</artifactId> + <version>3.0.0-SNAPSHOT</version> + </parent> + + <artifactId>unomi-persistence-elasticsearch-conditions</artifactId> + <name>Apache Unomi :: Persistence :: ElasticSearch :: Conditions</name> + <description>Conditions ElasticSearch persistence implementation for the Apache Unomi Context Server</description> + <packaging>bundle</packaging> + + <dependencyManagement> + <dependencies> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-bom</artifactId> + <version>${project.version}</version> + <type>pom</type> + <scope>import</scope> + </dependency> + </dependencies> + </dependencyManagement> + <dependencies> + <dependency> + <groupId>org.osgi</groupId> + <artifactId>osgi.core</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-api</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-common</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-persistence-spi</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.google.guava</groupId> + <artifactId>guava</artifactId> + <version>${guava.version}</version> + </dependency> + + <dependency> + <groupId>com.fasterxml.jackson.core</groupId> + <artifactId>jackson-databind</artifactId> + </dependency> + + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-lang3</artifactId> + </dependency> + <dependency> + <groupId>commons-collections</groupId> + <artifactId>commons-collections</artifactId> + </dependency> + <dependency> + <groupId>commons-io</groupId> + <artifactId>commons-io</artifactId> + </dependency> + <dependency> + <groupId>co.elastic.clients</groupId> + <artifactId>elasticsearch-java</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.elasticsearch.client</groupId> + <artifactId>elasticsearch-rest-client</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-metrics</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + + <dependency> + <groupId>junit</groupId> + <artifactId>junit</artifactId> + <scope>test</scope> + </dependency> + <dependency> + <groupId>com.hazelcast</groupId> + <artifactId>hazelcast-all</artifactId> + <version>3.12.8</version> + <scope>provided</scope> + </dependency> Review Comment: Did you try to remove hazelcast dependency. Maybe with ES 9 client it is no more needed. If yes, remove it but also from BOM and parent. ########## itests/pom.xml: ########## @@ -223,7 +225,7 @@ <xpack.ml.enabled>false</xpack.ml.enabled> <path.repo>${project.build.directory}/snapshots_repository</path.repo> <cluster.routing.allocation.disk.threshold_enabled>false</cluster.routing.allocation.disk.threshold_enabled> - <http.cors.allow-origin>*</http.cors.allow-origin> + <!--http.cors.allow-origin>*</http.cors.allow-origin--> Review Comment: Is it commented for test or because it is no more needed. Maybe we can remove it if no more needed. ########## itests/src/test/java/org/apache/unomi/itests/SegmentIT.java: ########## @@ -563,7 +563,7 @@ public void testScoringRecalculation() throws Exception { // insure the profile is engaged; try { Map<String, Object> pastEvent = ((List<Map<String, Object>>)testEvent.getProfile().getSystemProperties().get("pastEvents")).stream().filter(profilePastEvent -> profilePastEvent.get("key").equals(pastEventCondition.getParameterValues().get("generatedPropertyKey"))).findFirst().get(); - Assert.assertEquals("Profile should have 2 events in the scoring", 2, (long) pastEvent.get("count")); + Assert. assertEquals("Profile should have 2 events in the scoring", 2, (long) pastEvent.get("count")); Review Comment: It seems that space is not wanted ########## package/pom.xml: ########## @@ -318,7 +301,7 @@ <feature>unomi-kar</feature> <feature>unomi-router-karaf-feature</feature> <feature>unomi-groovy-actions</feature> - <feature>unomi-rest-ui</feature> + <!--<feature>unomi-rest-ui</feature>--> Review Comment: Is there is a reason to remove rest-ui or is it a merge mistake ? ########## persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/querybuilders/core/SourceEventPropertyConditionESQueryBuilder.java: ########## @@ -33,31 +32,31 @@ public class SourceEventPropertyConditionESQueryBuilder implements ConditionESQu public SourceEventPropertyConditionESQueryBuilder() { } - private void appendFilderIfPropExist(List<QueryBuilder> queryBuilders, Condition condition, String prop){ + private void appendFilderIfPropExist(List<Query> queryBuilders, Condition condition, String prop) { final Object parameter = condition.getParameter(prop); if (parameter != null && !"".equals(parameter)) { - queryBuilders.add(QueryBuilders.termQuery("source." + prop, (String) parameter)); + queryBuilders.add(Query.of(q -> q.term(t -> t.field("source." + prop).value(v -> v.stringValue((String) parameter))))); } } - public QueryBuilder buildQuery(Condition condition, Map<String, Object> context, ConditionESQueryBuilderDispatcher dispatcher) { - List<QueryBuilder> queryBuilders = new ArrayList<QueryBuilder>(); - for (String prop : new String[]{"id", "path", "scope", "type"}){ + public Query buildQuery(Condition condition, Map<String, Object> context, ConditionESQueryBuilderDispatcher dispatcher) { + List<Query> queryBuilders = new ArrayList<Query>(); + for (String prop : new String[]{"id", "path", "scope", "type"}) { Review Comment: Same renaming here queryBuilders -> queries ########## persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/querybuilders/core/SourceEventPropertyConditionESQueryBuilder.java: ########## @@ -33,31 +32,31 @@ public class SourceEventPropertyConditionESQueryBuilder implements ConditionESQu public SourceEventPropertyConditionESQueryBuilder() { } - private void appendFilderIfPropExist(List<QueryBuilder> queryBuilders, Condition condition, String prop){ + private void appendFilderIfPropExist(List<Query> queryBuilders, Condition condition, String prop) { final Object parameter = condition.getParameter(prop); if (parameter != null && !"".equals(parameter)) { - queryBuilders.add(QueryBuilders.termQuery("source." + prop, (String) parameter)); + queryBuilders.add(Query.of(q -> q.term(t -> t.field("source." + prop).value(v -> v.stringValue((String) parameter))))); } } - public QueryBuilder buildQuery(Condition condition, Map<String, Object> context, ConditionESQueryBuilderDispatcher dispatcher) { - List<QueryBuilder> queryBuilders = new ArrayList<QueryBuilder>(); - for (String prop : new String[]{"id", "path", "scope", "type"}){ + public Query buildQuery(Condition condition, Map<String, Object> context, ConditionESQueryBuilderDispatcher dispatcher) { + List<Query> queryBuilders = new ArrayList<Query>(); + for (String prop : new String[]{"id", "path", "scope", "type"}) { appendFilderIfPropExist(queryBuilders, condition, prop); } if (queryBuilders.size() >= 1) { if (queryBuilders.size() == 1) { return queryBuilders.get(0); } else { - BoolQueryBuilder boolQueryBuilder = QueryBuilders.boolQuery(); - for (QueryBuilder queryBuilder : queryBuilders) { + BoolQuery.Builder boolQueryBuilder = new BoolQuery.Builder(); + for (Query queryBuilder : queryBuilders) { boolQueryBuilder.must(queryBuilder); Review Comment: and queryBuilder -> query ########## persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/querybuilders/core/SourceEventPropertyConditionESQueryBuilder.java: ########## @@ -33,31 +32,31 @@ public class SourceEventPropertyConditionESQueryBuilder implements ConditionESQu public SourceEventPropertyConditionESQueryBuilder() { } - private void appendFilderIfPropExist(List<QueryBuilder> queryBuilders, Condition condition, String prop){ + private void appendFilderIfPropExist(List<Query> queryBuilders, Condition condition, String prop) { Review Comment: maybe queryBuilders could be renamed queries as now there is no more QueryBuilder. ########## persistence-spi/pom.xml: ########## @@ -76,12 +77,28 @@ <artifactId>commons-collections</artifactId> <scope>provided</scope> </dependency> + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-lang3</artifactId> + <scope>provided</scope> + </dependency> <dependency> <groupId>org.slf4j</groupId> <artifactId>slf4j-api</artifactId> <scope>provided</scope> </dependency> - + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-scripting</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-metrics</artifactId> + <version>${project.version}</version> + <scope>provided</scope> Review Comment: unomi module dependencies should be placed on top of dependency section and version is not necessary because included by BOM, if not, add module in BOM to avoid specifying version here. -- 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]
