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.

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

GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage #7323

Merged
merged 12 commits into from Jun 29, 2022
Merged

GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage #7323

merged 12 commits into from Jun 29, 2022

Conversation

mivanac
Copy link
Contributor

@mivanac mivanac commented Jan 28, 2022 •
edited

Implement logic, that for each dispatcher thread, in case events are unsuccessfully dispatched, when marking them as possible duplicate, for same batch of events notify secondary bucket.

Remove current logic, to mark all events in bucket, when it becomes primary.

For more info see https://cwiki.apache.org/confluence/display/GEODE/Improve+possible+duplicate+logic+for+secondary+buckets+in+WAN

For all changes:

  • [*] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • [*] Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • [*] Is your initial contribution a single, squashed commit?

  • [*] Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

@mivanac mivanac marked this pull request as ready for review February 15, 2022 12:25
DonalEvans
DonalEvans requested changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@DonalEvans DonalEvans left a comment

Choose a reason for hiding this comment

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

This PR doesn't introduce any tests to cover the new behaviour, and the RFC linked to in the ticket is still in draft mode. This PR should be put into draft mode too, until discussion of the RFC is complete, then marked ready for review once the implementation matches what was finally decided on in the RFC and has sufficient test coverage.

@mivanac mivanac marked this pull request as draft February 16, 2022 06:49
@mivanac mivanac force-pushed the feature/GEODE-9997 branch 2 times, most recently from 3dd0c75 to 0b72ed6 Compare February 22, 2022 19:55
@mivanac mivanac changed the title GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage to signal … GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage #7323 Feb 23, 2022
@mivanac mivanac changed the title GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage #7323 GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage Feb 23, 2022
@mivanac mivanac marked this pull request as ready for review February 24, 2022 07:32
onichols-pivotal
onichols-pivotal reviewed Feb 24, 2022
View reviewed changes
Copy link
Contributor

@onichols-pivotal onichols-pivotal left a comment •
edited

Choose a reason for hiding this comment

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

comment deleted

@mivanac mivanac marked this pull request as draft March 2, 2022 20:36
@mivanac mivanac force-pushed the feature/GEODE-9997 branch 5 times, most recently from b4daf08 to 4ab1496 Compare March 17, 2022 12:28
@mivanac mivanac requested a review from kirklund April 26, 2022 14:47
@mivanac
Copy link
Contributor Author

mivanac commented May 3, 2022

Hi @kirklund ,
any comment on made changes.

Thanks

@mivanac
Copy link
Contributor Author

mivanac commented May 9, 2022

Hi @kirklund , are changes OK with you?

@mivanac
Copy link
Contributor Author

mivanac commented May 18, 2022

Hi @kirklund , are changes OK with you?
It have past 22 days since changes that you requested are made, or answered.
Thanks.

@mivanac
Copy link
Contributor Author

mivanac commented May 23, 2022

Hi @kirklund , are changes OK with you?

@mivanac
Copy link
Contributor Author

mivanac commented May 25, 2022

Hi @upthewaterspout, @agingade and @dschneider-pivotal
could you review this PR.

Thanks

@mivanac mivanac dismissed kirklund’s stale review May 26, 2022 06:26

Since for one month there is no feedback/review of requested changes.

kirklund
kirklund previously requested changes Jun 2, 2022
View reviewed changes
geode-core/src/main/java/org/apache/geode/cache/wan/GatewaySender.java Outdated
Comment on lines 244 to 245
@Experimental
void prepareForStop();
Copy link
Contributor

Choose a reason for hiding this comment

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

All experimental APIs should have javadoc and annotation explanation strings. Please see MetricsPublishingService as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since changes are only for internal use, impacts moved to InternalGatewaySender interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kirklund , are changes OK with you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kirklund , are changes now OK?

@mivanac mivanac requested a review from kirklund June 7, 2022 18:34
@mivanac mivanac dismissed kirklund’s stale review June 29, 2022 20:37

Since API changes are removed, and modifications are not reviewed for 27 days, I will dismiss CR

@mivanac mivanac merged commit ef7dc45 into apache:develop Jun 29, 2022
mkevo pushed a commit to Nordix/geode that referenced this pull request Sep 7, 2022
@mivanac
GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage (apache#7323)
b96c216 
* GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage to signal duplicate events on secondary buckets
mkevo pushed a commit to Nordix/geode that referenced this pull request Oct 27, 2022
@mivanac @mkevo
GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage (apache#7323)
ec5d3b7 
* GEODE-9997: added ParallelQueueSetPossibleDuplicateMessage to signal duplicate events on secondary buckets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onichols-pivotal onichols-pivotal onichols-pivotal left review comments

@jake-at-work jake-at-work jake-at-work approved these changes

@jinmeiliao jinmeiliao jinmeiliao approved these changes

@DonalEvans DonalEvans DonalEvans approved these changes

@gesterzhou gesterzhou Awaiting requested review from gesterzhou

@boglesby boglesby Awaiting requested review from boglesby

@nabarunnag nabarunnag Awaiting requested review from nabarunnag

@jchen21 jchen21 Awaiting requested review from jchen21

@echobravopapa echobravopapa Awaiting requested review from echobravopapa

@Bill Bill Awaiting requested review from Bill

@kamilla1201 kamilla1201 Awaiting requested review from kamilla1201

@upthewaterspout upthewaterspout Awaiting requested review from upthewaterspout

@dschneider-pivotal dschneider-pivotal Awaiting requested review from dschneider-pivotal

@agingade agingade Awaiting requested review from agingade

@albertogpz albertogpz Awaiting requested review from albertogpz

@jdeppe-pivotal jdeppe-pivotal Awaiting requested review from jdeppe-pivotal

@kirklund kirklund Awaiting requested review from kirklund

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@mivanac @kirklund @jake-at-work @jinmeiliao @DonalEvans @onichols-pivotal