GEODE-1897: Initial refactor of CreateRegionCommand #956
Conversation
| public ExpirationAttributes(int expirationTime, ExpirationAction expirationAction) { | ||
| this.timeout = expirationTime; | ||
| this.action = expirationAction; | ||
| if (expirationTime < 0) { |
There was a problem hiding this comment.
I noticed that the javadoc says this method should throw an IllegalArgumentException if the expirationTime is nonpositive. We should either update the javadoc or throw the exception in that case.
| CliStrings.ALTER_REGION__MSG__SPECIFY_VALID_CLASSNAME_FOR_CACHELISTENER_0_IS_INVALID, | ||
| new Object[] {cacheListener})); | ||
| } | ||
| cacheLoader = convertDefaultValue(cacheLoader, StringUtils.EMPTY); |
There was a problem hiding this comment.
I would have expected the @CliOption parameters to deal with these default values. Do you know if there is a reason we did it here instead?
| if (r != null) | ||
| return r; | ||
| do { | ||
| // String command = "create region --name="+key+" --type="+defaultRegionType; |
There was a problem hiding this comment.
This looks like it was left around by accident.
| assertThat(args.getEntryExpirationTTL()).isNull(); | ||
| assertThat(args.getRegionExpirationIdleTime()).isNull(); | ||
| assertThat(args.getRegionExpirationTTL()).isNull(); | ||
| // |
There was a problem hiding this comment.
Looks like this comment got left behind
| } | ||
| } | ||
|
|
||
| // CommandExecutor executor = new CommandExecutor(); |
There was a problem hiding this comment.
This looks like it was left around by accident.
- Also updated sanctionedSerializables
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.concurrent.CopyOnWriteArraySet; | ||
|
|
There was a problem hiding this comment.
Why was import ordering changed. Our standard has java.* classes preceding org.apache.geode classes.
pdxrunner
left a comment
pdxrunner
left a comment
There was a problem hiding this comment.
I lot of the changed files have reorder imports. Please insure that they are all standardized
| int initialCapacity = 16; | ||
| float loadFactor = 0.75f; | ||
| int concurrencyLevel = 16; | ||
| int concurrencyLevel = DEFAULT_CONCURRENCY_LEVEL; |
There was a problem hiding this comment.
Why only define symbolic constant for concurrencyLevel? For consistency I'd either define constants for all the unexplained hardcoded numbers (initialCapacity & loadFactor ?) or none of them. DEFAULT_CONCURRENCY is only used the once so I don't see a reason for singling it out.
There was a problem hiding this comment.
I wanted to be able to reference this value outside of this class. I guess the other values aren't being used/referenced anywhere outside of this class.
Also, the AttributesFactory class is deprecated, so making larger changes is less important I think.
|
|
||
| import java.io.DataInput; | ||
| import java.io.DataOutput; | ||
| import java.io.IOException; |
There was a problem hiding this comment.
Import ordering - see my comment on AttributesFactory
| public String getCacheWriterClass() { | ||
| return cacheWriterClass; | ||
| } | ||
| } |
There was a problem hiding this comment.
+1 I like the removal of the inner class
|
|
||
| @Test | ||
| public void testRegionExistsReturnsCorrectValue() throws Exception { | ||
| InternalCache cache = mock(InternalCache.class); |
There was a problem hiding this comment.
Delete as you now have cache as in instance variable that's set in the @before
4 participants
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.