Skip to content
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.

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

Add includeValue support for MultiMap#addLocalEntryListener #18815

Merged
merged 6 commits into from
Jan 19, 2022

Conversation

kathapatel
Copy link
Contributor

@kathapatel kathapatel commented May 31, 2021

Enabled includeValue support for Multimap#addLocalEntryListener, implemented listner in MultiMapProxyImpl and ClientMultiMapProxy.

Fixes #18548

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

  • API
  • client protocol format
  • serialized form
  • snapshot format

Checklist:

  • Labels (Team:, Type:, Source:, Module:) and Milestone set
  • Add Add to Release Notes label if changes should be mentioned in release notes or Not Release Notes content if changes are not relevant for release notes
  • Request reviewers if possible
  • New public APIs have @Nonnull/@Nullable annotations
  • New public APIs have @since tags in Javadoc
  • Send backports/forwardports if fix needs to be applied to past/future releases

@kathapatel kathapatel requested a review from a team as a code owner May 31, 2021 13:21
@hz-devops-test hz-devops-test added the Source: Community PR or issue was opened by a community user label May 31, 2021
@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

devOpsHazelcast commented May 31, 2021

CLA assistant check
All committers have signed the CLA.

@kathapatel
Copy link
Contributor Author

@ihsandemir It's my first PR, could you please help to confirm on remaining 2 items of the checklist?

Copy link
Contributor

@gurbuzali gurbuzali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also add tests for the newly added methd, you can take a look at MultiMapListenerTest class

@kathapatel kathapatel requested a review from gurbuzali June 4, 2021 05:18
@srknzl
Copy link
Member

srknzl commented Jun 4, 2021

@kathapatel Regarding checklist, I think this should be added to release notes since a new API is introduced. Also this should be backward and forward ported to other versions. Correct me if I'm wrong @ihsandemir @gurbuzali

@kathapatel
Copy link
Contributor Author

@srknzl @gurbuzali Do we need to backport/forward port? if so could you please guide me?

@ihsandemir
Copy link
Contributor

@kathapatel Regarding checklist, I think this should be added to release notes since a new API is introduced. Also this should be backward and forward ported to other versions. Correct me if I'm wrong @ihsandemir @gurbuzali

@srknzl This is a new API, hence, I do not think we need any backport.

@ihsandemir
Copy link
Contributor

verify

@ihsandemir ihsandemir added Source: Community PR or issue was opened by a community user and removed Source: Community PR or issue was opened by a community user labels Jun 14, 2021
@AyberkSorgun AyberkSorgun modified the milestones: 5.0, 5.1 Jul 12, 2021
@Holmistr Holmistr removed the request for review from gurbuzali December 14, 2021 10:20
@Holmistr
Copy link
Contributor

@ihsandemir I see that the requested changes have been addressed. Could you give it another look?

@ahmetmircik
Copy link
Member

ahmetmircik commented Jan 18, 2022

For the main issue #18548,
I think true behavior was broken at here. True behavior should be including values for local listeners by default in UUID addLocalEntryListener(@Nonnull EntryListener<K, V> listener) method. IMap addLocalEntryListener is also including values by default. So my suggestion is just to restore broken behavior by including values in published events.
BTW, the new method introduced in this PR can also be merged as an enhancement.

WDYT? @ihsandemir @vbekiaris

Update:
Apparently i am wrong to think it was include-value false previously. Thanks @ihsandemir for pointing it.

Copy link
Member

@ahmetmircik ahmetmircik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ahmetmircik
Copy link
Member

run-lab-run

@hz-devops-test
Copy link

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]   PartitionIndexingTest.testOnProgrammaticallyAddedIndexes:168->assertPartitionsIndexedCorrectly:220 map_hash_this is missing 2 partitions expected:<101> but was:<99>
[INFO] 
[ERROR] Tests run: 46696, Failures: 1, Errors: 0, Skipped: 1015
[INFO] 

[ERROR] There are test failures.

@ahmetmircik
Copy link
Member

run-lab-run

@ahmetmircik
Copy link
Member

@kathapatel thanks for the contribution.

@ahmetmircik ahmetmircik changed the title includeValue support for Multimap#addLocalEntryListener Add includeValue support for MultiMap#addLocalEntryListener Jan 19, 2022
@ahmetmircik ahmetmircik merged commit 6bf2146 into hazelcast:master Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multimap#addLocalEntryListener does not support including the values
9 participants