[GitHub] [geode] kirklund commented on a change in pull request #5188: GEODE-8099: add dlock around cms create/delete operations.

[GitBox <gi...@apache.org>]

kirklund commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r436081052



##########
File path: geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorClusterManagementServiceIntegrationTest.java
##########
@@ -56,13 +57,38 @@ public void tearDown() {
     }
   }
 
+  @Before
+  public void setup() throws URISyntaxException {

Review comment:
       I recommend moving all the field and var creations at the top, then move all the when blocks together, order both blocks in some way (I alphabetize), and then consider moving the subject(s) under test to the bottom of the setup method.
   
   I also recommend cleaning up any statics in tearDown that you've configured in setUp. For example: how to undo this statement in tearDown:
   ```
   BaseManagementService.setManagementService(cache, managementService);
   ```
   The statics will still have this configured after each test completes.

##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##########
@@ -114,6 +122,7 @@
   private final MemberValidator memberValidator;
   private final CommonConfigurationValidator commonValidator;
   private final InternalCache cache;
+  private DistributedLockService cmsDlockService;

Review comment:
       Let's move away from lazy-initializing and mutable fields and just make it final.
   
   I'm assuming you made it mutable for testing. This is where you typically want to chain more than one constructor. One of the constructors needs `@VisibleForTesting` and pass in the DistributedLockService (as a mock or whatever). The main public constructor can then create the DistributedLockService and pass it as a parameter to the next constructor. The test would use the next constructor that accepts a DistributedLockService.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org