GEODE-3955: Add AEQ and Gateway Sender information to 'describe region' output. by PurelyApplied · Pull Request #1099 · apache/geode

PurelyApplied · 2017-11-29T00:26:15Z

I feel like the non-trivial refactoring deserves its own commit, if only for ease of review in this pull request.

My inexperience with Mockito is showing through and I got a little carried away in the RegionDescriptionJUnitTest. I don't know if the spy on its consumption in DescribeRegionCommand is beyond the scope of a reasonable JUnit and/or if we have (or should have) proper coverage of that in wider-scope tests. I don't really like how I dug into the command output JSON. So on the one hand, I don't really like those tests and could see them getting deleted. But on the other hand, it is more test coverage in the cheap JUnit category. Thoughts?

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 build run 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.

* Cleanup of DescribeRegionCommand for better readability and testability. * Flattened unnecessarily-nested logic blocks * Removed dead class fields from RegionDescription, RegionAttributesDefault * GetRegionDescriptionFunction implements Function instead of extending deprecated FunctionAdapter * Added waitTilGatewaySendersAreReady to MemberVM, MemberStarterRule

…n' output. * Add DUnit test coverage for 'describe region' command * Add JUnit test coverage for RegionDescription class

pdxrunner · 2017-11-29T16:55:41Z


 package org.apache.geode.test.dunit.rules;

+import static org.awaitility.Awaitility.await;


Unused import

pdxrunner · 2017-11-29T16:55:56Z

+
 import java.io.File;
 import java.util.Arrays;
+import java.util.concurrent.TimeUnit;


Unused import

pdxrunner · 2017-11-29T16:56:09Z


 import org.apache.commons.io.FileUtils;

+import org.apache.geode.management.DistributedSystemMXBean;


Unused import

I could've sworn I pushed the buttons! I have become my own worst enemy.

pdxrunner · 2017-11-29T17:01:40Z


-
-    if (partitionAttributes != null)
+    if (partitionAttributes != null) {


+! Excellent refactoring!

jinmeiliao · 2017-11-29T17:30:17Z

+  }
+
+  @SuppressWarnings("deprecation")
+  private static void configureServers(MemberVM server1, MemberVM server2) {


just for easy understanding of what this test is doing, we could probably get rid of this entire call. We are not describing any of the regions created in here, right?

Sure could! Another instance of not being judicious in what I copy when pulling something from another class.

jinmeiliao · 2017-11-29T17:31:07Z

+    props.setProperty(REMOTE_LOCATORS, "localhost[" + sending_locator.getPort() + "]");
+    lsRule.startLocatorVM(2, props);
+
+    MemberVM server1 = lsRule.startServerVM(3, "group1", sending_locator.getPort());


do we need to have grouping for our servers? if we are not asserting it, we don't need to do that.

jinmeiliao · 2017-11-29T17:33:04Z

+  }
+
+  @Test
+  public void describeRegionWithGatewayAndAsyncEventQueue() throws Exception {


since we have only one tests here, let's put the system setup in this test. If any future tests needs to be added, it's up to them to put any common setup in a @before tag. This would make it easy to spot what the test is doing.

jinmeiliao · 2017-11-29T17:38:21Z

+  }
+
+  private JSONObject getSharedAttributedJson(CommandResult commandResult) {
+    return commandResult.getContent().getInternalJsonObject().getJSONObject("__sections__-0")


use commandResult.getTableContent instead of this private method.

jinmeiliao · 2017-11-29T17:38:33Z

+        .getJSONObject("__sections__-0").getJSONObject("__tables__-0").getJSONObject("content");
+  }
+
+  private JSONObject getMemberSpecificAttributeJson(CommandResult commandResult) {


use commandResult.getTableContent

I will add some logic to the CommandResult class to go digging into the structure, since it seems to belong to that more than this class. Unfortunately, I can't just use getTableContent because the structure here is significantly different than in other commands, notably that this commandResult has two __section__-x levels before the tables.

jinmeiliao · 2017-11-29T17:49:54Z

In your Junit test, looks like you are extending the test to what's done in the DescribeRegionCommand. Let's get together to pair on wring a unit test for the command only.

pdxrunner · 2017-11-29T17:52:37Z

+    comparisonMap.put(partKeyShared, partValueShared);
+    comparisonMap.put(regKeyShared, regValueShared);
+    commonMap.put(evictionKeyB, evictionValueB);
+    commonMap.put(regKeyA, regValueB);


Move these commonMap.put's to group with the ones above - or did you intend for them to be put into the comparisonMap?

Totally meant them to be the other map. Good catch.

pdxrunner · 2017-11-29T17:55:19Z

+  }
+
+  @Test
+  public void findCommonRemovesDisagreeingKeys() {


A better name might be "findCommonRemovesKeysWithDisagreeingValues"

Sure would be!

pdxrunner · 2017-11-29T18:10:37Z

+
+  public static final String regionName = "mockRegion1";
+
+  private static final DescribeRegionCommand command = spy(DescribeRegionCommand.class);


It's not necessary to spy this, just define "command = new DescribeRegionCommand()".

The spy is really only necessary if you're specifying some test action using "when()" or using "verify" to makesure specific calls are made.

Thanks. Still learning the ropes when it comes to Mockito.

I've refactored these into its own class and extended the mocking around DescribeRegionCommand, since it's really not part of the RegionDescription testing anyway.

pdxrunner · 2017-11-29T18:35:23Z

+import org.apache.geode.test.junit.categories.DistributedTest;
+import org.apache.geode.test.junit.rules.GfshCommandRule;
+
+@Category(DistributedTest.class)


Should this be a DUnit or an Acceptance test?

I could be wrong, but I had thought that line was drawn between DUnit tests using LocatorServerStartupRule and AcceptanceTests using GfshRule.

Right. I guess I wasn't very clear - what I meant was look at what is being tested. This seems to be an overall end-to-end test that might fit better as an Acceptance test.

Oh, that makes more sense.

Especially since most of the test is done via gfsh.executeAndAssertThat, it should be a pretty simple thing to convert this into a GfshScript.of(...). The only thing I wouldn't know how to convert is the guards that wait until the async event queues and the gateway senders are live. I don't know that those checks could work in an AcceptanceTest, since that would belong to a separate process.

Other than the "sleep for a while" anti-pattern, I don't see how to safely guarantee that the AEQ and Gateway are live before proceeding.

…nJUnit to their own JUnit class. Expand and clean existing tests.

…re .add is called. Remove various 'this.' in .add() for consistency with rest of method.

… and RegionAttributesInfo

jinmeiliao · 2017-12-05T04:12:36Z

-   */
-  private static final long serialVersionUID = 1L;

+  private static final long serialVersionUID = 336184564012988487L;


Just curious, why do we need to change to this? if need to be changed, why this complicated number? why not 2L?

I believe it's just a convention. It's not mentioned in any of the Google Java Style Guide that we've adopted as our own.

It needs to be changed to correctly deserialize, but I think you're right -- it could be changed to any not-the-previous-value-1 and be valid.

Looking around online, there doesn't seem to be any consensus on increment vs rehash the value. The best I could find was that a big, long number catches the eye and reminds future developers of the importance of considering backwards incompatibility.

jdeppe-pivotal · 2017-11-29T22:02:42Z

-        LogWrapper.getInstance().info(t.getMessage(), t);
+    // Log any errors received.
+    resultList.stream().filter(Throwable.class::isInstance).map(Throwable.class::cast)
+        .forEach(t -> LogWrapper.getInstance().info(t.getMessage(), t));


I don't think this should be using LogWrapper - isn't that only for immediate gfsh use? Rather LogService.getLogger().

…n' output. * Add DUnit test coverage for 'describe region' command * Add JUnit test coverage for RegionDescription class * Expansion and refactoring of touched classes. - This closes #1099.

PurelyApplied added 2 commits November 28, 2017 16:19

GEODE-3955: Add AEQ and Gateway Sender information to 'describe regio…

5b7b1ed

…n' output. * Add DUnit test coverage for 'describe region' command * Add JUnit test coverage for RegionDescription class

PurelyApplied requested review from jdeppe-pivotal, jinmeiliao and pdxrunner November 29, 2017 00:27

pdxrunner reviewed Nov 29, 2017

View reviewed changes

jinmeiliao requested changes Nov 29, 2017

View reviewed changes

pdxrunner reviewed Nov 29, 2017

View reviewed changes

pdxrunner requested changes Nov 29, 2017

View reviewed changes

pdxrunner reviewed Nov 29, 2017

View reviewed changes

PurelyApplied added 3 commits November 29, 2017 13:59

Move JUnit tests touching DescribeRegionCommand from RegionDescriptio…

ef85b44

…nJUnit to their own JUnit class. Expand and clean existing tests.

suggested edits

dd70620

Add subsection logic to CommandResult.getTableContent.

3bc9a02

pdxrunner approved these changes Nov 30, 2017

View reviewed changes

Instantiate field maps at declaration to avoid NPE if referenced befo…

264f7bc

…re .add is called. Remove various 'this.' in .add() for consistency with rest of method.

jinmeiliao approved these changes Nov 30, 2017

View reviewed changes

PurelyApplied added 2 commits December 4, 2017 14:34

Updated sanctioned serializables to include changed RegionDescription…

0a3d29a

… and RegionAttributesInfo

Update serialVersionUIDs and sanctionedSerializables.

ecda1bb

pdxrunner approved these changes Dec 4, 2017

View reviewed changes

jinmeiliao reviewed Dec 5, 2017

View reviewed changes

jdeppe-pivotal reviewed Dec 6, 2017

View reviewed changes

PurelyApplied added 3 commits December 6, 2017 09:43

LogWrapper -> LogService logger.

6bba7c0

Merge branch 'develop' into geode-3955

3d249c7

Merge stagger, spotless

c427d40

PurelyApplied closed this Dec 6, 2017

PurelyApplied deleted the geode-3955 branch December 6, 2017 22:38


		package org.apache.geode.test.dunit.rules;

		import static org.awaitility.Awaitility.await;


		import org.apache.commons.io.FileUtils;

		import org.apache.geode.management.DistributedSystemMXBean;



		if (partitionAttributes != null)
		if (partitionAttributes != null) {


		public static final String regionName = "mockRegion1";

		private static final DescribeRegionCommand command = spy(DescribeRegionCommand.class);

Uh oh!

Conversation

PurelyApplied commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For all changes:

Note:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinmeiliao commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PurelyApplied commented Nov 29, 2017 •

edited

Loading