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

Cleanup leftovers for WAN replication SPI #16052

Merged
merged 4 commits into from Nov 26, 2019

Conversation

@mmedenjak
Copy link
Contributor

mmedenjak commented Nov 18, 2019

  • removed Node parameter from WanReplicationConsumer. Node is private
    API and implementations may implement HazelcastInstanceAware to get
    something like configuration.
  • changed type of AbstractWanPublisherConfig.implementation to
    WanReplicationPublisher and WanConsumerConfig.implementation to
    WanReplicationConsumer.
  • moved republishReplicationEvent, publishAntiEntropyEvent,
    pause/stop/resume, getStats and removeWanEvents methods from public API.
    These methods are all related to our WAN implementation (WAN event
    republication, anti-entropy, lifecycle methods, statistics and clearing
    the WAN data) and the user need not implement these methods to have the
    simplest functioning WAN replication. Some of these are still exposed as
    REST URIs (clear, pause/stop/resume, sync...) but the REST handlers will
    be no-op in the case the user provides a custom publisher implementation.
    It may be possible we expose these methods as public SPI in the future
    so users may try to implement it and integrate with the REST handlers
    and other subsystems.
  • copied most migration and replication-related code from EE
    WanReplicationService to OS. This now allows the user to fully implement
    all methods on WAN publisher SPI, regardless if they are using OS or EE.
  • moved all migration and replication-related methods to a
    MigrationAwareWanReplicationPublisher interface. Now, if the user
    wants the publisher to participate in migrations and replication, they
    will need to implement this interface in addition to the
    WanReplicationPublisher. Because of this, WanReplicationPublisher is
    even simpler to implement.

Leftovers:

  • code duplication between OS and EE migration and replication related
    code. This can be addressed in a minor release.
  • exposing event data as public methods on WAN replication events

EE: hazelcast/hazelcast-enterprise#3363

@mmedenjak mmedenjak added this to the 4.0 milestone Nov 18, 2019
@mmedenjak mmedenjak requested review from ahmetmircik and blazember Nov 18, 2019
@mmedenjak mmedenjak self-assigned this Nov 18, 2019
}

@Override
public boolean isKnownServiceNamespace(ServiceNamespace namespace) {

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Nov 19, 2019

Member

why is this for only map-service? Can you please add a short explanatory javaDoc on this method?

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Nov 19, 2019

Author Contributor

On OS side, we only generate WAN events for map service. I'll add the javadoc.

Copy link
Member

ahmetmircik left a comment

overall LGTM, only added minor comments

@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-wan-publisher-spi-leftovers branch from 229a021 to e819852 Nov 19, 2019
for (Entry<String, Object> publisherEventContainer : eventContainersByPublisherId.entrySet()) {
String publisherId = publisherEventContainer.getKey();
Object eventContainer = publisherEventContainer.getValue();
WanReplicationPublisher publisher = service.getPublisherOrFail(wanReplicationScheme, publisherId);

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Nov 19, 2019

Member

What if we fail during getPublisherOrFail call in the middle of for loop? In that case, we can have some publishers called method processEventContainerReplicationData and some others are skipped.
Is this problematic or is it better to check first if we can have a fail-able publisher and crash accordingly? So is it worth to separate logic in this class into 2 steps like this: validate publishers and process them?

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Nov 19, 2019

Author Contributor

We haven't encountered that issue before (I guess that would probably mean the configuration is different between members) but yes, we can do this to improve the situation.

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Nov 26, 2019

Author Contributor

Added the check, you can take a look at the last commit.

Copy link
Contributor

blazember left a comment

Nice cleanup 👍

Matko Medenjak added 3 commits Nov 18, 2019
- removed Node parameter from WanReplicationConsumer. Node is private
API and implementations may implement HazelcastInstanceAware to get
something like configuration.
- changed type of AbstractWanPublisherConfig.implementation to
WanReplicationPublisher and WanConsumerConfig.implementation to
WanReplicationConsumer.
- moved republishReplicationEvent, publishAntiEntropyEvent,
pause/stop/resume, getStats and removeWanEvents methods from public API.
These methods are all related to our WAN implementation (WAN event
republication, anti-entropy, lifecycle methods, statistics and clearing
the WAN data) and the user need not implement these methods to have the
simplest functioning WAN replication. Some of these are still exposed as
REST URIs (clear, pause/stop/resume, sync...) but the REST handlers will
be no-op in the case the user provides a custom publisher implementation.
It may be possible we expose these methods as public SPI in the future
so users may try to implement it and integrate with the REST handlers
and other subsystems.
- copied most migration and replication-related code from EE
WanReplicationService to OS. This now allows the user to fully implement
all methods on WAN publisher SPI, regardless if they are using OS or EE.
- moved all migration and replication-related methods to a
MigrationAwareWanReplicationPublisher interface. Now, if the user
wants the publisher to participate in migrations and replication, they
will need to implement this interface in addition to the
WanReplicationPublisher. Because of this, WanReplicationPublisher is
even simpler to implement.

Leftovers:
- code duplication between OS and EE migration and replication related
code. This can be addressed in a minor release.
- exposing event data as public methods on WAN replication events
@hazelcast hazelcast deleted a comment from blazember Nov 26, 2019
@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-wan-publisher-spi-leftovers branch from e819852 to 5cd3695 Nov 26, 2019
@mmedenjak

This comment has been minimized.

Copy link
Contributor Author

mmedenjak commented Nov 26, 2019

Merging with unrelated test failure. Thank you for the reviews, guys!

@mmedenjak mmedenjak merged commit 7b1755d into hazelcast:master Nov 26, 2019
1 check failed
1 check failed
default Test `FAIL`ed. http://jenkins.hazelcast.com/job/Hazelcast-pr-builder/4674/
Details
@mmedenjak mmedenjak deleted the mmedenjak:4.0-wan-publisher-spi-leftovers branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.