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

Additional EWR features #7107

Merged
merged 1 commit into from Dec 23, 2015

Conversation

Projects
None yet
6 participants
@emrahkocaman
Copy link
Contributor

commented Dec 18, 2015

@emrahkocaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2015

run-lab-run

@enesakar

This comment has been minimized.

Copy link
Member

commented Dec 18, 2015

@pveentjer can you review this PR asap? we need this for EA3 release

long batchMaxDelayMillis = getLongValue("batch-max-delay-millis", getTextContent(targetChild));
wanTarget.setBatchMaxDelayMillis(batchMaxDelayMillis);
} else if ("queue-capacity".equals(targetChildName)) {
int queueCapacity = getIntegerValue("queueCapacity", getTextContent(targetChild));

This comment has been minimized.

Copy link
@bilalyasar

bilalyasar Dec 18, 2015

Collaborator

getIntegerValue("queueCapacity"... or getIntegerValue("queue-capacity"... ?

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 18, 2015

Author Contributor

Should be queue-capacity

private void handleWanTargetConfig(WanReplicationConfig wanReplicationConfig,
WanTargetClusterConfig wanTarget, Node targetChild) {
final String targetChildName = cleanNodeName(targetChild);
if ("replication-impl".equals(targetChildName)) {

This comment has been minimized.

Copy link
@bilalyasar

bilalyasar Dec 18, 2015

Collaborator

what if there is no class, fail-fast? what do you think @mesutcelik ?

This comment has been minimized.

Copy link
@mesutcelik

mesutcelik Dec 18, 2015

Contributor

we can set a default version....

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 18, 2015

Author Contributor

Node start will fail with ClassNotFoundException in that case. But I think this part is out of scope of this PR since there is nothing new here. I just extracted them to a method, logic is same as before.

This comment has been minimized.

Copy link
@mesutcelik

mesutcelik Dec 18, 2015

Contributor

I think that is generic problem with all configs that accept fully qualified class names... @bilalyasar , can you please create an issue for that?

*
* @return the {@link WanQueueFullBehavior} found or null if not found
*/
public static WanQueueFullBehavior getById(final int id) {

This comment has been minimized.

Copy link
@mesutcelik

mesutcelik Dec 18, 2015

Contributor

getById seems not used...

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 18, 2015

Author Contributor

In fact, ID is totally unnecessary since this config is not serialized. I'll remove.

/**
* Instruct WAN repl. impl to drop new events when WAN event queues are full
*/
DISCARD_AFTER_MUTATION(0),

This comment has been minimized.

Copy link
@mesutcelik

mesutcelik Dec 18, 2015

Contributor

what does DISCARD_AFTER_MUTATION mean?

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 18, 2015

Author Contributor

Actually this is the existing behavior of WAN replication and will be default behavior of the new version also.
When DISCARD_AFTER_MUTATION is chosen, mutating operation (put/remove/etc...) will be applied but WAN event will be discarded if WAN replication queue is full.

When THROW_EXCEPTION is chosen, a queue size check will be performed in beforeRun of mutating operations and WANReplicationQueueFullException will be throwed if WAN replication queue is full, therefore mutating operation (put/remove/etc...) won't run.

This comment has been minimized.

Copy link
@mesutcelik

mesutcelik Dec 18, 2015

Contributor

ok

private static final int DEFAULT_BATCH_SIZE = 500;
private static final long DEFAULT_BATCH_MAX_DELAY_MILLIS = 1000;
private static final int DEFAULT_QUEUE_CAPACITY = 10000;
private static final long DEFAULT_RESPONSE_TIMEOUT_MILLIS = 60000;

This comment has been minimized.

Copy link
@mesutcelik

mesutcelik Dec 18, 2015

Contributor

I see those final statics not used anywhere else so what is the point to declare them here and then assing to class fields?

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 18, 2015

Author Contributor

Checkstyle doesn't allow magic numbers.

This comment has been minimized.

Copy link
@mesutcelik

mesutcelik Dec 18, 2015

Contributor

ok. I would prefer to create a constructor which is accepting all class fields and remove all setters. Default constructor would use those static fields. However, that is my preference...

@@ -68,6 +68,14 @@ public PutAllOperation(String name, MapEntries mapEntries, boolean initialLoad)
}

@Override
public void beforeRun() throws Exception {
super.beforeRun();
if (shouldWanReplicate()) {

This comment has been minimized.

Copy link
@mesutcelik

mesutcelik Dec 18, 2015

Contributor

What is the diff between mapContainer.isWanReplicationEnabled() and shouldWanReplicate?

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 18, 2015

Author Contributor

Seems like they are same. It's possibly a leftover from times before isWanReplicationEnabled method is introduced.

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 18, 2015

Author Contributor

I'll remove shouldWanReplicate method.

@@ -37,6 +37,13 @@ public CacheGetAndRemoveOperation(String name, Data key, int completionId) {
}

@Override
protected void beforeRunInternal() {
if (cache.isWanReplicationEnabled()) {

This comment has been minimized.

Copy link
@serkan-ozal

serkan-ozal Dec 18, 2015

Contributor

not important but super.beforeRunInternal(); is needed here also?

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 18, 2015

Author Contributor

👍 I think so.

super.beforeRun();
if (mapContainer.isWanReplicationEnabled()) {
mapContainer.checkWanReplicationQueues();
}

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Dec 18, 2015

Member

We have a MapOperation to centralize this kind of checks.

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 18, 2015

Author Contributor

But we need to make this check only in mutating operations. Otherwise, for example a get operation will also throw exception when WAN queue is full.

@emrahkocaman emrahkocaman force-pushed the emrahkocaman:ewr-overload-handling branch from 071719f to ea083e7 Dec 22, 2015

@emrahkocaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

I've updated the PR.

For map, @ahmetmircik, I've moved WAN checks to WANAwareOperationProvider and cleaned beforeRun methods.

For cache, @serkan-ozal , I've also cleaned up WAN related checks from cache operations. They'll be available in ee only.

Also I've added tests for configuration part.

Guys, can you please check again?

@emrahkocaman emrahkocaman force-pushed the emrahkocaman:ewr-overload-handling branch 2 times, most recently from 8837253 to 6258967 Dec 22, 2015

}

/**
* Creates {@link MapOperationProvider} instance and wraps it into a {@link WANAwareOperationProvider} if

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Dec 22, 2015

Member

if seems leftover in the end.

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 22, 2015

Author Contributor

👍

*/
public abstract class MapOperationProviderDelegator implements MapOperationProvider {

abstract MapOperationProvider getOperationProviderDelegate();

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Dec 22, 2015

Member

Maybe renamed as getDelegate(), we know this is map-operation-provider from context.

This comment has been minimized.

Copy link
@emrahkocaman

emrahkocaman Dec 22, 2015

Author Contributor

I'll update.

@ahmetmircik

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

👍 with minor comments.

@emrahkocaman emrahkocaman force-pushed the emrahkocaman:ewr-overload-handling branch from 6258967 to c467d8c Dec 22, 2015

@emrahkocaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2015

run-lab-run

@mesutcelik

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2015

👍

mesutcelik added a commit that referenced this pull request Dec 23, 2015

@mesutcelik mesutcelik merged commit 0cd4bb6 into hazelcast:master Dec 23, 2015

1 check passed

default 10255 tests run, 44 skipped, 0 failed.
Details

@emrahkocaman emrahkocaman deleted the emrahkocaman:ewr-overload-handling branch Dec 7, 2016

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.