kirklund commented on a change in pull request #7440: URL: https://github.com/apache/geode/pull/7440#discussion_r824941591
########## File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/RebalanceWhileCreatingRegionDistributedTest.java ########## @@ -0,0 +1,298 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.internal.cache; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.Serializable; +import java.util.stream.IntStream; +import java.util.stream.Stream; + +import org.apache.logging.log4j.Logger; +import org.junit.Rule; +import org.junit.Test; + +import org.apache.geode.cache.PartitionAttributesFactory; +import org.apache.geode.cache.Region; +import org.apache.geode.cache.RegionFactory; +import org.apache.geode.cache.RegionShortcut; +import org.apache.geode.distributed.internal.ClusterDistributionManager; +import org.apache.geode.distributed.internal.DistributionMessage; +import org.apache.geode.distributed.internal.DistributionMessageObserver; +import org.apache.geode.distributed.internal.InternalDistributedSystem; +import org.apache.geode.distributed.internal.membership.InternalDistributedMember; +import org.apache.geode.internal.cache.partitioned.RemoveBucketMessage; +import org.apache.geode.logging.internal.log4j.api.LogService; +import org.apache.geode.test.dunit.AsyncInvocation; +import org.apache.geode.test.dunit.rules.ClientVM; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.dunit.rules.DistributedBlackboard; +import org.apache.geode.test.dunit.rules.MemberVM; +import org.apache.geode.test.junit.rules.serializable.SerializableTestName; + +public class RebalanceWhileCreatingRegionDistributedTest implements Serializable { Review comment: Test looks ok, but my personal preference is to write this with `DistributedReference<ServerLauncher>` rule and use `AtomicReference<CountDownLatch>` instead of the blackboard rule. Just use the default Locator for dunit unless you need to configure it and interact with it -- in which case you can also use `DistributedReference<LocatorLauncher>`. The reasons for using actual Geode APIs directly: 1) a test should use the same APIs that Users have to use to guarantee that it's doing what a user does 2) test authors learn the User APIs and become intimately familiar with their shortcomings 3) other developers can instantly see how servers are configured when maintaining the test in the future without having to dig through testing helper classes 4) the real product User API becomes the only API that needs to be maintained or changed 5) facilitates moving to JUnit 5 or other testing frameworks I won't request any changes, but I also won't approve. Please consider the above points for future PRs. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
