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

WAN publisher SPI cleanup, part 2 #15195

Merged
merged 1 commit into from Jun 26, 2019

Conversation

@mmedenjak
Copy link
Contributor

mmedenjak commented Jun 18, 2019

This change separates private and public WAN API so it's mostly import
changes. After the API is separated, we can more easily detect leaking
internals (e.g. the EWRMigrationContainer parameter in
WanReplicationEndpoint.collectReplicationData). This will help us fix
any methods that leak internals and finally to join the OS and EE
WAN SPI into one.

Note to reviewers: you can use git show | grep -E "^[-+][^-+]+" | grep -v "[-+]import" on the command line to list all of the changes that were not import changes. You'll have to correlate them to the exact file by either searching the "Files changed" in GitHub or by searching git show.

The public WAN packages and classes (in OS and EE):

com.hazelcast.cache.wan:
	CacheWanEventFilter
com.hazelcast.map.wan:
	MapWanEventFilter
com.hazelcast.enterprise.wan:
	EnterpriseReplicationEventObject
	WanConsistencyCheckEvent
	WanEventQueueMigrationListener
	WanFilterEventType
	WanReplicationConsumer
	WanReplicationEndpoint
	WanSyncEvent
	WanSyncType
com.hazelcast.wan:
	ConsistencyCheckResult
	DistributedServiceWanEventCounters
	ReplicationEventObject
	WanReplicationEndpoint
	WanReplicationEvent
	WanReplicationPublisher
	WANReplicationQueueFullException
	WanSyncStats

Also moved ICache operations to impl package (some of them were WAN operations).

Remaining cases to inspect and fix:

  • whether com.hazelcast.enterprise.wan.impl.replication.WanBatchReplication should remain private or public (since the user needs to list it in the configuration). Alternatively, we can add an alias (or make it the default) so the user doesn't need to list the whole class name.
  • whether to move ReplicationSupportingService out of SPI since we are
    closing off custom service SPI
  • usage of EWRMigrationContainer in
    WanReplicationEndpoint#collectReplicationData
  • need to expose both sync and consistency check methods instead of
    current WanReplicationEndpoint#publishSyncEvent
  • all mentioning of "queue" in the SPI/API
  • simplify various putBackup, publishReplicationEvent,
    publishReplicationEventBackup, publishReplicationEvent methods
  • rename checkWanReplicationQueues, WANQueueFullBehavior,
    WANReplicationQueueFullException to avoid mentioning queue
  • join ReplicationEventObject and EnterpriseReplicationEventObject
  • join WanReplicationEndpoint from OS and EE

Fixes: hazelcast/hazelcast-enterprise#2168
EE: hazelcast/hazelcast-enterprise#3048

@@ -100,7 +100,7 @@
<hz:property name="maxEndpoints">2</hz:property>
</hz:properties>
</hz:wan-publisher>
<hz:wan-publisher group-name="ankara" class-name="com.hazelcast.enterprise.wan.replication.WanBatchReplication">
<hz:wan-publisher group-name="ankara" class-name="com.hazelcast.enterprise.wan.impl.replication.WanBatchReplication">

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Jun 25, 2019

Member

Referencing an impl class doesn't seem ideal. If we never expect a publisher implemented by user, hiding it completely can be a good option.

This comment has been minimized.

Copy link
@blazember

blazember Jun 26, 2019

Contributor

I think even if the user can implement it, this class can be resolved internally without forcing the user to set it. This class is not even designed for extension, so 👍 for making it private.

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Jun 26, 2019

Author Contributor

Yes, it was one of the things that I was a bit concerned about but not too much. For one, it definitely is internal in the sense that we don't support the user to instantiating it using new com.hazelcast.enterprise.wan.impl.replication.WanBatchReplication(); and using it somewhere in their code. It's our internal implementation and it's satisfactory for most users using WAN.

The problem here may be that we require the fully qualified class name. I was thinking that we might add some aliases like WanBatchReplication so it's easier to configure and easier on the eyes. The other idea was simply to make it the default as most users will use it anyway. So this could work then:

<hz:wan-publisher group-name="ankara">
...
</hz:wan-publisher>

But either of these ideas would be part of a subsequent PR, not this one. WDYT?

This comment has been minimized.

Copy link
@blazember

blazember Jun 26, 2019

Contributor

One more thought. Maybe instead of the FQCN of the endpoint implementation, we should accept a factory's FQCN. So that additional dependencies can be injected in the way the user likes.

@ahmetmircik

This comment has been minimized.

Copy link
Member

ahmetmircik commented Jun 25, 2019

rebase?

@ahmetmircik

This comment has been minimized.

Copy link
Member

ahmetmircik commented Jun 25, 2019

simplify various putBackup, publishReplicationEvent,
publishReplicationEventBackup, publishReplicationEvent methods

can same be applied to WanReplicationPublisher interface? Maybe we can reduce it to a one method interface:

interface WanReplicationPublisher {
    void publishReplicationEvent(String serviceName, ReplicationEventObject eventObject);
}
@blazember

This comment has been minimized.

Copy link
Contributor

blazember commented Jun 26, 2019

rename checkWanReplicationQueues, WANQueueFullBehavior,
WANReplicationQueueFullException to avoid mentioning queue

I think the latter two are specific to our EE implementation and they are in line with that implementation's internals and naming, so maybe we can keep them as they are. We should move the full behavior configuration to the properties though together with the consistency check strategy.

Maybe we can rename the consistency-check-strategy configuration to something like anti-entropy-strategy or synchronization-method.

@mmedenjak

This comment has been minimized.

Copy link
Contributor Author

mmedenjak commented Jun 26, 2019

can same be applied to WanReplicationPublisher interface? Maybe we can reduce it to a one method interface:

@ahmetmircik yes, that would be great. Unfortunately there are some use cases where we cannot avoid the additional parameter of boolean isBackup so we must additionally have methods accepting that parameter. You might remember, you had a PR which was determining if the event was a primary or backup based on the event key and the partition table which was great and eliminated this parameter but it turned out the partition table might be stale in some cases. Other methods may have other problems why they can't be merged into a single method. I'll try and have a closer look at each but I can't guarantee that I'll be able to get it down to a single method for publication.

I think the latter two are specific to our EE implementation and they are in line with that implementation's internals and naming, so maybe we can keep them as they are. We should move the full behavior configuration to the properties though together with the consistency check strategy.
Maybe we can rename the consistency-check-strategy configuration to something like anti-entropy-strategy or synchronization-method.

@blazember yes, agree, some attributes should probably be moved to properties as they are implementation-specific (some implementations might not be queue-based).

Matko Medenjak
This change separates private and public WAN API so it's mostly import
changes. After the API is separated, we can more easily detect leaking
internals (e.g. the EWRMigrationContainer parameter in
WanReplicationEndpoint.collectReplicationData). This will help us fix
any methods that leak internals and finally to join the OS and EE
WAN SPI into one.

The public WAN packages and classes (in OS and EE):
com.hazelcast.cache.wan:
	CacheWanEventFilter
com.hazelcast.map.wan:
	MapWanEventFilter
com.hazelcast.enterprise.wan:
	EnterpriseReplicationEventObject
	WanConsistencyCheckEvent
	WanEventQueueMigrationListener
	WanFilterEventType
	WanReplicationConsumer
	WanReplicationEndpoint
	WanSyncEvent
	WanSyncType
com.hazelcast.wan:
	ConsistencyCheckResult
	DistributedServiceWanEventCounters
	ReplicationEventObject
	WanReplicationEndpoint
	WanReplicationEvent
	WanReplicationPublisher
	WANReplicationQueueFullException
	WanSyncStats

Remaining cases to inspect and fix:
- whether to move ReplicationSupportingService out of SPI since we are
closing off custom service SPI
- usage of EWRMigrationContainer in
WanReplicationEndpoint#collectReplicationData
- need to expose both sync and consistency check methods instead of
current WanReplicationEndpoint#publishSyncEvent
- all mentioning of "queue" in the SPI/API
- simplify various putBackup, publishReplicationEvent,
publishReplicationEventBackup, publishReplicationEvent methods
- rename checkWanReplicationQueues, WANQueueFullBehavior,
WANReplicationQueueFullException to avoid mentioning queue
- join ReplicationEventObject and EnterpriseReplicationEventObject
- join WanReplicationEndpoint from OS and EE

Fixes: https://github.com/hazelcast/hazelcast-enterprise/issues/2168
@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-wan-publisher-spi-2 branch from 177c41c to 355718a Jun 26, 2019
@mmedenjak mmedenjak merged commit 8ef57e7 into hazelcast:master Jun 26, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@mmedenjak mmedenjak deleted the mmedenjak:4.0-wan-publisher-spi-2 branch Jun 26, 2019
@mmedenjak mmedenjak self-assigned this Sep 24, 2019
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.