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.

Sign up for GitHub

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

Jump to bottom

ISPN-4160 Do not allow the state transfer chunk size to be <= 0 #2496

Closed
wants to merge 1 commit into from
Closed

ISPN-4160 Do not allow the state transfer chunk size to be <= 0 #2496

wants to merge 1 commit into from

Conversation

vblagoje
Copy link

Master only. Dan should review. See discussion at https://issues.jboss.org/browse/ISPN-4160

@danberindei
Copy link
Member

I'm getting a failure in ReplStateTransferCacheLoaderTest:

12:30:30,868 ERROR (remote-thread-NodeA-p11404-t1:) [InboundInvocationHandlerImpl] ISPN000260: Exception executing command
java.lang.IllegalArgumentException: stateTransferChunkSize must be greater than 0
at org.infinispan.statetransfer.OutboundTransferTask.<init>(OutboundTransferTask.java:96)
at org.infinispan.statetransfer.StateProviderImpl.startOutboundTransfer(StateProviderImpl.java:285)
at org.infinispan.statetransfer.StateRequestCommand.perform(StateRequestCommand.java:69)
at org.infinispan.remoting.InboundInvocationHandlerImpl.handleInternal(InboundInvocationHandlerImpl.java:95)

danberindei
danberindei reviewed Apr 16, 2014
View reviewed changes
core/src/main/java/org/infinispan/configuration/cache/StateTransferConfiguration.java
@@ -63,7 +63,7 @@ public StateTransferConfiguration timeout(long l) {

/**
* If &gt; 0, the state will be transferred in batches of {@code chunkSize} cache entries.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for an if here, as the chunk size must be > 0 all the time.

@danberindei
Copy link
Member

Looks like ClusterConfigurationBuilder.validate() doesn't call stateTransferConfigurationBuilder.validate(), that's why the error only appears when a state transfer task is started.

@vblagoje
Copy link
Author

Ok, let me correct this one. Thanks Dan

danberindei
danberindei reviewed Apr 23, 2014
View reviewed changes
core/src/test/java/org/infinispan/api/MixedModeTest.java
@@ -18,9 +18,13 @@ protected void createCacheManagers() throws Throwable {
ConfigurationBuilder invalAsync = getDefaultClusteredCacheConfig(CacheMode.INVALIDATION_ASYNC, false);
ConfigurationBuilder local = getDefaultClusteredCacheConfig(CacheMode.LOCAL, false);

invalSync.clustering().stateTransfer().fetchInMemoryState(false);
invalAsync.clustering().stateTransfer().fetchInMemoryState(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vblagoje Actually, would you mind removing the lines in StateTransferConfigurationBuilder that throw an exception when state transfer is enabled in invalidation mode? I added a test for state transfer in invalidation mode some time ago (NonTxStateTransferInvalidationTest), but I forgot to remove that validation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danberindei I did, and therefore all of those clustering().stateTransfer().fetchInMemoryState(false) should be removed as well. Correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we should leave fetchInMemoryState enabled.

BTW, I'm still seeing a failure in StateTransferCacheLoaderTest: http://ci.infinispan.org/viewLog.html?buildId=7877&buildTypeId=bt9

@vblagoje
Copy link
Author

@danberindei forgot to mention that this one has been updated. Have a look

@danberindei
Copy link
Member

@vblagoje I still see the failure in CI, in ReplStateTransferCacheLoaderTest.createBeforeMethod

@vblagoje
Copy link
Author

Right! Corrected, apologies.

@vblagoje
Copy link
Author

vblagoje commented May 1, 2014

@danberindei ready for integration

tristantarrant
tristantarrant reviewed May 9, 2014
View reviewed changes
core/src/main/java/org/infinispan/configuration/cache/StateTransferConfigurationBuilder.java
throw new CacheConfigurationException(
"Cache cannot use INVALIDATION mode and have fetchInMemoryState set to true.");
if (chunkSize <= 0) {
throw new CacheConfigurationException("chunkSize can not be <= 0");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer exceptions such as these to use Log

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry Tristan, too late! We'll try to keep it in mind for the next time :)

@danberindei
Copy link
Member

Integrated, thanks Vladimir!

@danberindei danberindei closed this May 9, 2014
@vblagoje vblagoje deleted the t_4160 branch May 26, 2014 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@danberindei danberindei

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@vblagoje @danberindei @tristantarrant