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

Join WAN OS and EE SPI #15527

Merged
merged 5 commits into from Sep 18, 2019
Merged

Conversation

@mmedenjak
Copy link
Contributor

mmedenjak commented Sep 7, 2019

Performed further cleanup of WAN custom publisher SPI.
Removed WanReplicationEndpoint both in OS nor EE. It was unclear what
this interface was exactly for, how it was exactly different from
WanReplicationPublisher and if the user should implement
WanReplicationPublisher or WanReplicationEndpoint.
There is now a single interface which contains all of the methods to be
implemented by a custom publisher and it's located in OS -
WanReplicationPublisher.

Methods on WanReplicationPublisher used only in EE have a default
implementation so OS implementations need not implement those methods.

Merged WanReplicationPublisherDelegate from OS and EE.
WanReplicationPublisherDelegate no longer implements
WanReplicationPublisher as it is a special case of an internal
publisher which only delegates to other publisher implementations.
Having it not implement WanReplicationPublisher makes it simpler as
some methods it would inherit from the interface would not be invoked
at all and would further introduce confusion. There is only a single
implementation of WanReplicationPublisher that we provide in Hazelcast
Enterprise - WanBatchReplication.

Replaced WanReplicationPublisher#publishSyncEvent with
WanReplicationPublisher#publishAntiEntropyEvent. The anti-entropy
event is a parent for the sync event as also a consistency check can be
an anti-entropy event. The only reason we didn't do this in 3.x was
backwards compatibility.

Removed an internal interface EnterpriseReplicationEventObject
containing private API on enterprise WAN replication events and replaced
it with InternalWanReplicationEvent interface containing private API
for internal use by our WAN implementations.

Introduced InternalWanReplicationPublisher for private API exposed by
our WAN publisher implementations.

Removed WanReplicationPublisher#putBackup. It was only delegating to
publishReplicationEventBackup and with its' removal we now only have
three publication methods which makes it very simple for
implementation - publishReplicationEvent for events on partition
owner, publishReplicationEventBackup for events on partition backup
and republishReplicationEvent for republishing events received from
another cluster.

Moved method removeWanEvents(int, String, String, int) to
InternalWanReplicationPublisher as it's only used from our internal
WAN publisher implementation and not other parts of the Hazelcast
system. Custom implementations don't need to implement or invoke this
method.

Removed Node parameter from WanReplicationPublisher#init as it is private API. Implementations may now implement HazelcastInstanceAware to get a reference to the current instance.

Only missing part is exposing entry data on the WanReplicationEvent
which does not involve private API for custom implementations to use.

EE: hazelcast/hazelcast-enterprise#3146

@mmedenjak mmedenjak added this to the 4.0 milestone Sep 7, 2019
@mmedenjak mmedenjak self-assigned this Sep 7, 2019
@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-wan-publisher-spi-6 branch from 13b206c to aa3151f Sep 8, 2019
@mmedenjak mmedenjak changed the title [WIP] Join WAN OS and EE SPI Join WAN OS and EE SPI Sep 8, 2019
@mmedenjak mmedenjak marked this pull request as ready for review Sep 8, 2019
@mmedenjak mmedenjak requested review from blazember and ahmetmircik Sep 9, 2019
@@ -49,14 +91,147 @@
void republishReplicationEvent(WanReplicationEvent wanReplicationEvent);

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Sep 10, 2019

Member

method name raises the question: can't i do republishing by calling publishReplicationEvent twice?

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Sep 13, 2019

Author Contributor

Maybe the method name is a bit misleading but there's a fundamental difference between publishReplicationEvent and republishReplicationEvent. publishReplicationEvent is supposed to be called on the source cluster on the partition owner where the entry has actually been mutated while republishReplicationEvent is called on the target cluster on any random member receiving a WAN batch. Republishing then might involve some stuff like finding the partition owner and forwarding the events to it to be published.

We might also rename republishReplicationEvent to something like republishReceivedWanReplicationEvent or something simpler. I don't have any ideas at the moment.

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Sep 16, 2019

Member

forwardReceivedReplicationEvent?

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Sep 16, 2019

Author Contributor

Sure. We have to keep in mind that we actually have a config property isRepublishingEnabled :

/**
* Returns {@code true} if incoming WAN events to this member should be
* republished (forwarded) to this WAN replication reference.
*/
public boolean isRepublishingEnabled() {
return republishingEnabled;
}

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Sep 16, 2019

Member

if it is not a big hassle, both can be changed but decision is yours.

*
* @param mapName the map name
*/
void destroyMapData(String mapName);

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Sep 10, 2019

Member

is this an imap specific method?

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Sep 13, 2019

Author Contributor

Yes, it is. It's for cleaning up some publisher data once a map is destroyed.

* @param partitionId the partition ID of the WAN events should be removed
* @param count the number of events to remove
*/
int removeWanEvents(int partitionId, String serviceName, String objectName, int count);

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Sep 10, 2019

Member

can these all removeWanEvents be handled in a shared method?

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Sep 13, 2019

Author Contributor

Possibly, yes. I haven't yet tried doing so, maybe in a follow-up PR.

Matko Medenjak added 2 commits Sep 8, 2019
Removed `WanReplicationEndpoint` both in OS nor EE. It was unclear what
this interface was exactly for, how it was exactly different from
`WanReplicationPublisher` and if the user should implement
`WanReplicationPublisher` or `WanReplicationEndpoint`.
There is now a single interface which contains all of the methods to be
implemented by a custom publisher and it's located in OS -
`WanReplicationPublisher`.

Methods on `WanReplicationPublisher` used only in EE have a default
implementation so OS implementations need not implement those methods.

Merged `WanReplicationPublisherDelegate` from OS and EE.
`WanReplicationPublisherDelegate` no longer implements
`WanReplicationPublisher` as it is a special case of an internal
publisher which only delegates to other publisher implementations.
Having it not implement `WanReplicationPublisher` makes it simpler as
some methods it would inherit from the interface would not be invoked
at all and would further introduce confusion. There is only a single
implementation of `WanReplicationPublisher` that we provide in Hazelcast
Enterprise - `WanBatchReplication`.

Replaced `WanReplicationPublisher#publishSyncEvent` with
`WanReplicationPublisher#publishAntiEntropyEvent`. The anti-entropy
event is a parent for the sync event as also a consistency check can be
 an anti-entropy event. The only reason we didn't do this in 3.x was
 backwards compatibility.

Removed an internal interface `EnterpriseReplicationEventObject`
containing private API on enterprise WAN replication events and replaced
it with `InternalWanReplicationEvent` interface containing private API
for internal use by our WAN implementations.

Introduced `InternalWanReplicationPublisher` for private API exposed by
our WAN publisher implementations.

Removed `WanReplicationPublisher#putBackup`. It was only delegating to
`publishReplicationEventBackup` and with its' removal we now only have
three publication methods which makes it very simple for
implementation - `publishReplicationEvent` for events on partition
owner, `publishReplicationEventBackup` for events on partition backup
and `republishReplicationEvent` for republishing events received from
another cluster.

Moved method `removeWanEvents(int, String, String, int)` to
`InternalWanReplicationPublisher` as it's only used from our internal
WAN publisher implementation and not other parts of the Hazelcast
system. Custom implementations don't need to implement or invoke this
method.

Only missing part is exposing entry data on the WanReplicationEvent
which does not involve private API for custom implementations to use.
Matko Medenjak
Rename WanReplicationService#clearQueues with "removeWanEvents"
Remove unused WanReplicationService#checkWanReplicationQueues
Replace some "target group name" usages with "WAN publisher ID"
Replace "wanGroupName" in cache operations with "origin"
Clean up some broken links in javadoc
Remove unused fields in WanCacheMergeOperation
@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-wan-publisher-spi-6 branch from aa3151f to d749ef0 Sep 16, 2019
@mmedenjak

This comment has been minimized.

Copy link
Contributor Author

mmedenjak commented Sep 16, 2019

@blazember @ahmetmircik added a separate commit and rebased on top of current master. Here's an overview:

  • Address review comments
  • Rename WanReplicationService#clearQueues with "removeWanEvents"
  • Remove unused WanReplicationService#checkWanReplicationQueues
  • Replace some "target group name" usages with "WAN publisher ID"
  • Replace "wanGroupName" in cache operations with "origin"
  • Clean up some broken links in javadoc
  • Remove unused fields in WanCacheMergeOperation

Unchanged:

  • kept method republishReceivedWanReplicationEvent. Not sure yet of the benefit of renaming it to forwardReceivedReplicationEvent, will think about it some more - #15527 (comment)
  • no action on InternalWanReplicationPublisher#destroyMapData
  • kept InternalWanReplicationPublisher#removeWanEvents. It is internal and merging various overloaded variants of removeWanEvents can be done in a separate PR - #15527 (comment)
  • WanReplicationConsumer#init still has a reference to Node. This will be changed in a separate PR.

Other leftovers:

  • move ReplicationSupportingService out of SPI
  • add entry key, value and event type accessors on WanReplicationEvent
  • move methods in DelegatingWanReplicationScheme used only from EE to EE

Please take a look.

@ahmetmircik ahmetmircik self-requested a review Sep 17, 2019
return publishers.get(publisherId);
}

public void addPublisher(@Nonnull String publisherId,

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Sep 18, 2019

Member

OS is not using this method. Can we move this kind of unused methods to EE? This class has lots of unused ones.

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Sep 18, 2019

Author Contributor

Again, I'm not too concerned with it as it's private and moving it elsewhere might create other kind of complexity. One idea is that we might have an OS WanReplicationPublishersContainer with some methods and then an EE EnterpriseWanReplicationPublishersContainer extending the OS one. But one point of this PR was to eliminate the OS-EE distinction. WDYT?

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Sep 18, 2019

Member

i think this is a minor point, no action is ok.

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Sep 18, 2019

Author Contributor

I'll leave this for now as I'd like to focus on getting as much breaking changes done until feature freeze. You are right about it, though and I'll add it to the list of todo's in the comment above.

Matko Medenjak
Matko Medenjak
@mmedenjak mmedenjak merged commit 2c27b16 into hazelcast:master Sep 18, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@mmedenjak mmedenjak deleted the mmedenjak:4.0-wan-publisher-spi-6 branch Sep 18, 2019
@mmedenjak

This comment has been minimized.

Copy link
Contributor Author

mmedenjak commented Sep 18, 2019

Thank you a million times for the reviews, guys! We are almost at the finish line with a perfect SPI :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.