gharris1727 commented on code in PR #16266:
URL: https://github.com/apache/kafka/pull/16266#discussion_r1635470153
##########
trogdor/src/main/java/org/apache/kafka/trogdor/workload/SustainedConnectionWorker.java:
##########
@@ -60,7 +60,7 @@
public class SustainedConnectionWorker implements TaskWorker {
private static final Logger log =
LoggerFactory.getLogger(SustainedConnectionWorker.class);
- private static final SystemTime SYSTEM_TIME = new SystemTime();
+ private static final Time SYSTEM_TIME = Time.SYSTEM;
Review Comment:
I think you can inline this variable.
##########
clients/src/main/java/org/apache/kafka/common/utils/Time.java:
##########
@@ -30,7 +30,40 @@
*/
public interface Time {
- Time SYSTEM = new SystemTime();
+ // A time implementation that uses the system clock and sleep call.
+ // Use inline implementation to ensure that only one Time#SYSTEM exists
in a program
+ Time SYSTEM = new Time() {
Review Comment:
I think rather than being an anonymous Time$1, it should still be a named
SystemTime.
You could leave the existing class in-place and make it package-local, or
make this an inner class.
##########
clients/src/test/java/org/apache/kafka/common/utils/TimeTest.java:
##########
@@ -80,4 +82,11 @@ public void testWaitObjectConditionSatisfied() throws
InterruptedException {
assertTrue(time.milliseconds() < deadlineMs);
assertNull(caughtException.get());
}
+
+ @Test
+ public void testOnlyOneSystemTime() {
Review Comment:
We don't need to test Java's variable finality, I think it is safe to assume
this will work.
In other words, this test would pass regardless of the implementation of the
Time interface, so it can't possibly detect any flaw in the code.
##########
clients/src/test/java/org/apache/kafka/common/utils/TimeTest.java:
##########
@@ -26,9 +26,11 @@
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
-public abstract class TimeTest {
Review Comment:
Is there a reason this needed to become concrete? I think the existing
pattern was fine, and made it clear that the suite was parameterized by the
createTime() method.
--
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]
