New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce multi partition predicate [HZ-1347] #21319

Merged

ahmetmircik merged 108 commits into hazelcast:master from software-is-art:master

Aug 25, 2022

Contributor

software-is-art commented Apr 26, 2022 •

edited

This is a redux of PR-18171. I'm continuing on from @ashley-taylor.

I've gone with a slightly different approach of making a backwards compatible change to the existing PartitionPredicate interface (instead of making an entirely new interface). This seemed lower risk as existing code with single partition keys will continue to work, while anything new can take advantage of the getPartitionKeys method.

Breaking changes (list specific methods/types/messages):

PartitionPredicateImpl constructor

Checklist:

Labels (Team:, Type:, Source:, Module:) and Milestone set
Label Add to Release Notes or Not Release Notes content set
Request reviewers if possible
Send backports/forwardports if fix needs to be applied to past/future releases
New public APIs have @Nonnull/@Nullable annotations
New public APIs have @since tags in Javadoc

software-is-art requested a review from a team as a code owner

April 26, 2022 22:23

hz-devops-test added the Source: Community label

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

10 similar comments

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022

Can one of the admins verify this patch?

Collaborator

devOpsHazelcast commented Apr 26, 2022 •

edited

All committers have signed the CLA.

software-is-art commented

View reviewed changes

hazelcast/src/main/java/com/hazelcast/query/impl/predicates/PartitionPredicateImpl.java Outdated Show resolved Hide resolved

vbekiaris added Type: Enhancement Team: Core Module: Query labels

Collaborator

vbekiaris commented Apr 27, 2022

run-lab-run

Collaborator

vbekiaris commented Apr 27, 2022

run-lts-compilers

hz-devops-test commented Apr 27, 2022

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[ERROR]   BinaryCompatibilityTest.readAndVerifyBinaries:127
[INFO] 
[ERROR] Tests run: 4353, Failures: 48, Errors: 0, Skipped: 8
[INFO] 
[ERROR] There are test failures.

Contributor Author

software-is-art commented Apr 27, 2022

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

I'll need to back out changes made to the binary compatibility test file, and go from there

Contributor Author

software-is-art commented May 1, 2022

@vbekiaris am I able to run the test suite using the same comments?

Contributor

Holmistr commented May 9, 2022

@software-is-art Unfortunately no, due to security reasons (classic cliche :) ). Let me trigger it.

Contributor

Holmistr commented May 9, 2022

run-lab-run

Holmistr mentioned this pull request

Ability to specify multiple partitions in a predicate query. #18171

Closed

Holmistr added this to the 5.2 Backlog milestone

Holmistr requested a review from ahmetmircik

May 9, 2022 11:25

Holmistr mentioned this pull request

When using InMemoryFormat.OBJECT Ability to mark data as immutable to remove seralisation cost on local operations #21198

Open

Contributor

Holmistr commented May 10, 2022

Related issue: #21319

Contributor Author

software-is-art commented Aug 9, 2022 •

edited

@ahmetmircik / @vbekiaris I've updated the serialization binary file for the compatibility tests.

We've dropped the Versioned interface from PartitionPredicateImpl as part of the PR feedback. I forgot that would cause issues with these tests.

Collaborator

vbekiaris commented Aug 9, 2022

run-lab-run

mafryer commented Aug 12, 2022

@ahmetmircik @vbekiaris is there anything more required for this PR?

Contributor

mdumandag commented Aug 12, 2022

Looking at the recent discussion and changes, I believe the most important one is this one. We should be using toDataWithStrategy not toData here. #21319 (comment)

Also, we have this one, #21319 (comment), but as Ahmet said, it can be done later in a separate PR


          * Use toDataWithStrategy for MapProxySupport

f4397e0

Contributor Author

software-is-art commented Aug 15, 2022 •

edited

Looking at the recent discussion and changes, I believe the most important one is this one. We should be using toDataWithStrategy not toData here. #21319 (comment)

Also, we have this one, #21319 (comment), but as Ahmet said, it can be done later in a separate PR

I added a support for using toDataWithStrategy on a partition id set @ahmetmircik / @mdumandag

Member

ahmetmircik commented Aug 15, 2022

run-lab-run

Member

ahmetmircik commented Aug 16, 2022

@mafryer please see this: #21319 (comment)

software-is-art requested review from ahmetmircik and mdumandag and removed request for ahmetmircik and mdumandag

August 19, 2022 03:13

ahmetmircik approved these changes

View reviewed changes

Member

ahmetmircik left a comment

LGTM, thanks for the PR and addressing reviews patiently.

Member

ahmetmircik commented Aug 23, 2022

run-lab-run

Member

ahmetmircik commented Aug 23, 2022

@mdumandag are you ok with latest state of this PR?

mdumandag reviewed

View reviewed changes

hazelcast/src/main/java/com/hazelcast/query/Predicates.java

+                   * @param <V>           the type of values the predicate operates on.
+                   * @throws NullPointerException     if partitionKeys or target predicate are {@code null}
+                   * @throws IllegalArgumentException if partitionkeys is an empty set
+                   */

Contributor

mdumandag Aug 23, 2022

Minor: We can add @since tag here

hazelcast/src/test/java/com/hazelcast/map/PartitionsPredicatePerformanceTest.java

+              @RunWith(HazelcastParallelClassRunner.class)
+              @Category({NightlyTest.class})
+              public class PartitionsPredicatePerformanceTest extends HazelcastTestSupport {

Contributor

mdumandag Aug 23, 2022

Minor: Maybe we can do this as well

Contributor

mdumandag commented Aug 23, 2022

@ahmetmircik The latest state of the PR looks good to me. I just need to confirm one thing before approving the PR. Will we include this in 5.2? If not, we need to update the protocol PR with the correct versioning, but if we are going to include it, it can stay as it is, and I can approve the PR

Member

ahmetmircik commented Aug 23, 2022

@mdumandag intention is to put this work in 5.2.

mdumandag approved these changes

View reviewed changes

Member

ahmetmircik commented Aug 23, 2022

run-ee-tests

hz-devops-test commented Aug 23, 2022

The job Hazelcast-pr-builder-ee-tests of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

--------------------------
---------SUMMARY----------
--------------------------
[ERROR] Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=2048m; support was removed in 8.0
--------------------------
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   BPlusTreeMemoryTest.testDisposeWhileConcurrentlyAccessing:304 java.lang.AssertionError
[ERROR]   DeviceOperationExecutorTest.testExecutorShutdown:113->assertThreadCount:190 expected:<5> but was:<41>
[INFO] 
[ERROR] Tests run: 10454, Failures: 2, Errors: 0, Skipped: 86
[INFO] 
[ERROR] There are test failures.

ahmetmircik changed the title ~~Ability to specify multiple partitions in a predicate query (redux) [HZ-1347]~~ Introduce multi partition predicate [HZ-1347]

ahmetmircik merged commit 047c4d3 into hazelcast:master

mdumandag added the Add to Release Notes label

mdumandag pushed a commit to hazelcast/hazelcast-client-protocol that referenced this pull request


          Binary changes for [PR-21319](hazelcast/hazelcast#21319) in hazelcast…

77e5e1c

…/hazelcast (#426)

* Added partitionKeysData of type List_Data to PagingPredicateHolder

Contributor Author

software-is-art commented Aug 26, 2022

LGTM, thanks for the PR and addressing reviews patiently.

Thanks @ahmetmircik for being patient with me as I worked through it all. Looking forward to upgrading to 5.2 when it's out.

All the best for the next release 👏

AyberkSorgun modified the milestones: 5.2 Backlog, 5.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment