UNOMI-901: Upgrade elasticsearch client#729
Conversation
366094f to
c0d09c2
Compare
df8c02e to
600a452
Compare
5a1bb1b to
e75dbc8
Compare
51c2d8b to
065c931
Compare
065c931 to
b3f53b6
Compare
| <dependency> | ||
| <groupId>joda-time</groupId> | ||
| <artifactId>joda-time</artifactId> | ||
| <version>2.12.7</version> |
There was a problem hiding this 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.
| <dependency> | ||
| <groupId>com.google.guava</groupId> | ||
| <artifactId>guava</artifactId> | ||
| <version>${guava.version}</version> |
There was a problem hiding this 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...
| <artifactId>hazelcast-all</artifactId> | ||
| <version>3.12.8</version> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this 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
Outdated
| <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--> |
There was a problem hiding this comment.
Is it commented for test or because it is no more needed.
Maybe we can remove it if no more needed.
| 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")); |
There was a problem hiding this comment.
It seems that space is not wanted
persistence-spi/pom.xml
Outdated
| <groupId>org.apache.unomi</groupId> | ||
| <artifactId>unomi-metrics</artifactId> | ||
| <version>${project.version}</version> | ||
| <scope>provided</scope> |
There was a problem hiding this 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.
| <dependency> | ||
| <groupId>org.elasticsearch.client</groupId> | ||
| <artifactId>elasticsearch-rest-client</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Maybe we can remove hazelcast from here too.
| } | ||
|
|
||
| private void appendFilderIfPropExist(List<QueryBuilder> queryBuilders, Condition condition, String prop){ | ||
| private void appendFilderIfPropExist(List<Query> queryBuilders, Condition condition, String prop) { |
There was a problem hiding this comment.
maybe queryBuilders could be renamed queries as now there is no more 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"}) { |
There was a problem hiding this comment.
Same renaming here queryBuilders -> queries
| for (QueryBuilder queryBuilder : queryBuilders) { | ||
| BoolQuery.Builder boolQueryBuilder = new BoolQuery.Builder(); | ||
| for (Query queryBuilder : queryBuilders) { | ||
| boolQueryBuilder.must(queryBuilder); |
There was a problem hiding this comment.
and queryBuilder -> query
This pull request introduces several notable improvements and updates across the codebase, primarily focusing on support for Elasticsearch 9, and documentation enhancements. The most significant changes include the addition of new dependencies for Elasticsearch 9, updates of the test snapshots, and the removal of deprecated API methods. There are also minor improvements to JavaDoc formatting and test adjustments.
Elasticsearch 9 Support and Dependency Updates:
unomi-persistence-elasticsearch-coreinbom/artifacts/pom.xmland updated the corresponding dependency inkar/pom.xmlto enable Elasticsearch 9 support.bom/pom.xmlto add dependencies for the new Elasticsearch Java client (elasticsearch-java) and to ensure compatibility by specifying both old and new Elasticsearch versions for different modules.elasticsearch-maven-pluginversion initests/pom.xmlfor improved compatibility.Test and Snapshot Updates:
itests/README.mdand related test files to usesnapshot_2instead of the oldersnapshot_1.6.x, reflecting a newer baseline for migration testing.API and Codebase Clean-up:
Documentation:
Minor Code Improvements:
<>) for generics inCondition.java.These changes collectively modernize the project’s dependencies, improve migration test robustness, and enhance code maintainability and documentation clarity.