KAFKA-16879 SystemTime should use singleton mode #16266
Conversation
SystemTime should use singleton mode
SystemTime should use singleton mode
SystemTime should use singleton mode--update UT
SystemTime should use singleton mode--update UT
SystemTime should use singleton mode
clients/src/test/java/org/apache/kafka/common/utils/TimeTest.java
Outdated
Show resolved
Hide resolved
clients/src/test/java/org/apache/kafka/common/utils/TimeTest.java
Outdated
Show resolved
Hide resolved
| @@ -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; | |||
There was a problem hiding this comment.
I think you can inline this variable.
There was a problem hiding this comment.
You are right,
After updating SystemTime to singleton mode, many class attributes time can be modified inline, but I am not sure whether to modify it because it is a large-scale change to modify many files.
There was a problem hiding this comment.
I don't mean the local variables and instance variables time, those can stay as-is, as they can be useful later if someone wants to replace the Time instance for the whole test.
I meant this duplicate static final variable. Either it should be an instance variable like the rest, or inlined.
There was a problem hiding this comment.
Thank you
I have made an update ， and update ConnectionStressWorker that also use static final Time time = Time.SYSTEM.
| @@ -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; | |||
There was a problem hiding this comment.
I don't mean the local variables and instance variables time, those can stay as-is, as they can be useful later if someone wants to replace the Time instance for the whole test.
I meant this duplicate static final variable. Either it should be an instance variable like the rest, or inlined.
| * A time implementation that uses the system clock and sleep call. Use `Time.SYSTEM` instead of creating an instance | ||
| * of this class. | ||
| * A time implementation that uses the system clock and sleep call. | ||
| * Every kafka process has a single instance of class <code>SystemTime</code> that allows the process to interface |
There was a problem hiding this comment.
nit: I think the mention of "process" here is more confusing than helpful. If you want to mention this is a singleton, say "Use the singleton Time.SYSTEM", or just leave the javadoc as-is.
There was a problem hiding this comment.
Thank you
I have restored it
SystemTime should use singleton mode
Currently, the SystemTime class, which provides system time-related functionalities such as getting the current timestamp 、sleep、and await can be instantiated multiple times.
Howerver, system time is unique，In an application, the time obtained in different places should be consistent, But now the time obtained by using the Java System class to interact with the underlying layer is the same。
So I suggest changing it to a singleton mode， reflect the uniqueness of system time in design
Use inline implementation to ensure there is only one SystemTime，avoid large-scale changes
Committer Checklist (excluded from commit message)