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

Improve WAN diagnostics #12165

Conversation

@mmedenjak
Copy link
Contributor

@mmedenjak mmedenjak commented Jan 17, 2018

  • reuses the StoreLatencyPlugin to provide latency tracking for WAN
    endpoints
  • added counters for WAN sent and received events

EE PR: hazelcast/hazelcast-enterprise#1878
Forward port: #11782

@mmedenjak mmedenjak added this to the 3.9.3 milestone Jan 17, 2018
@mmedenjak mmedenjak self-assigned this Jan 17, 2018
@mmedenjak mmedenjak requested a review from ahmetmircik Jan 17, 2018
@mmedenjak mmedenjak force-pushed the mmedenjak:wan-diagnostics-and-refactoring-backport branch from 0fb6de1 to dae6797 Jan 17, 2018
@gokhanoner gokhanoner self-requested a review Jan 17, 2018
@mmedenjak
Copy link
Contributor Author

@mmedenjak mmedenjak commented Jan 17, 2018

Failed run because of known issue: #11935

@mmedenjak
Copy link
Contributor Author

@mmedenjak mmedenjak commented Jan 18, 2018

@pveentjer @gokhanoner many thanks for reviewing the PR! Feel free to review the EE side if you have time - hazelcast/hazelcast-enterprise#1878

return new EventCounter();
}
};
private final ConcurrentHashMap<String, EventCounter> eventCounterMap = new ConcurrentHashMap<String, EventCounter>();

This comment has been minimized.

@ahmetmircik

ahmetmircik Jan 18, 2018
Member

Do we need to remove related EventCounter from eventCounterMap upon map destroy?

This comment has been minimized.

@mmedenjak

mmedenjak Jan 18, 2018
Author Contributor

Hm, might make sense 👍

This comment has been minimized.

@mmedenjak

mmedenjak Jan 26, 2018
Author Contributor

Done.

@@ -16,8 +16,28 @@

package com.hazelcast.wan;

import com.hazelcast.nio.serialization.Data;

/**
* Marker interface for WAN replication messages

This comment has been minimized.

@ahmetmircik

ahmetmircik Jan 18, 2018
Member

Seems this is not a marker any more.

And also cant we provide this event counting functionality out-of-the-box(assuming this interface is implemented by the end user since it is public). Is the new methods in this interface really required?

This comment has been minimized.

@mmedenjak

mmedenjak Jan 18, 2018
Author Contributor

You're right, I should move it out of the public interface.
The new methods were in the interface to avoid casting to a specific class when incrementing stats. I'll try to find a different way.

This comment has been minimized.

@mmedenjak

mmedenjak Jan 25, 2018
Author Contributor

I thought about it and we can freely add methods to this interface since it is not implemented by the user. I changed the javadoc not to say that it's a marker interface. I've kept the new methods which allow me to use polymorhism and have each event (and future events) increment its own count, instead of having a big if-else chain with instanceof.

OTOH when adding the counter removal on map/cache destroy, I found the current design unsatisfactory as the counters were located in classes which were supposed to be implemented by the user (e.g. implementations of WanReplicationPublisher). This is why I've reorganised it a bit and now all counters are located in the wan service.

Please take a look.

@mmedenjak mmedenjak requested a review from blazember Jan 25, 2018
@mmedenjak mmedenjak force-pushed the mmedenjak:wan-diagnostics-and-refactoring-backport branch from 16ed973 to 8431b9e Jan 25, 2018
- reuses the StoreLatencyPlugin to provide latency tracking for WAN
endpoints
- added counters for WAN sent and received events
@mmedenjak mmedenjak force-pushed the mmedenjak:wan-diagnostics-and-refactoring-backport branch from 8431b9e to cf55716 Jan 25, 2018
/**
* Marker interface for WAN replication messages
* Interface for all WAN replication messages
*/
public interface ReplicationEventObject {

This comment has been minimized.

@ahmetmircik

ahmetmircik Jan 26, 2018
Member

can we move this interface to impl package? or do we expect users to implement this?

This comment has been minimized.

@mmedenjak

mmedenjak Jan 26, 2018
Author Contributor

No, users do not implement it. I'll move it.

This comment has been minimized.

@mmedenjak

mmedenjak Jan 26, 2018
Author Contributor

Seems we can't move it since it's a method argument in WanReplicationPublisher which can be implemented by the user. Moving it would cause existing implementations of WanReplicationPublisher to not compile.

/**
* Thread safe container for {@link WanEventCounter} grouped by service name.
*/
public class WanEventCounterContainer {

This comment has been minimized.

@ahmetmircik

ahmetmircik Jan 26, 2018
Member

can we put this class into impl package?

This comment has been minimized.

@mmedenjak

mmedenjak Jan 26, 2018
Author Contributor

Moved to impl package.

WanEventCounter getReceivedEventCounter(String serviceName);

/**
* Returns a counter of received and processed WAN replication events.

This comment has been minimized.

@ahmetmircik

ahmetmircik Jan 26, 2018
Member

seems a copy&paste error in javaDoc?

This comment has been minimized.

@mmedenjak

mmedenjak Jan 26, 2018
Author Contributor

Fixed.

Copy link
Member

@ahmetmircik ahmetmircik left a comment

LGTM

- reuses the StoreLatencyPlugin to provide latency tracking for WAN
endpoints
- added counters for WAN sent and received events
@mmedenjak
Copy link
Contributor Author

@mmedenjak mmedenjak commented Jan 26, 2018

@ahmetmircik @gokhanoner @blazember thanks for the reviews! Please check out the EE PR: hazelcast/hazelcast-enterprise#1878

- reuses the StoreLatencyPlugin to provide latency tracking for WAN
endpoints
- added counters for WAN sent and received events
@mmedenjak mmedenjak merged commit 43aa03f into hazelcast:maintenance-3.x Jan 29, 2018
1 check passed
1 check passed
@devOpsHazelcast
default Test PASSed.
Details
@mmedenjak mmedenjak deleted the mmedenjak:wan-diagnostics-and-refactoring-backport branch Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants