[GitHub] [flink] azagrebin commented on a change in pull request #10348: [FLINK-14951][tests] Harden the thread safety of State TTL backend tests

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

azagrebin commented on a change in pull request #10348: [FLINK-14951][tests] Harden the thread safety of State TTL backend tests
URL: https://github.com/apache/flink/pull/10348#discussion_r355516266
 
 

 ##########
 File path: flink-end-to-end-tests/flink-stream-state-ttl-test/src/main/java/org/apache/flink/streaming/tests/MonotonicTTLTimeProvider.java
 ##########
 @@ -47,32 +48,24 @@
 	 * from RocksDB compaction filter threads.
 	 */
 
-	private static boolean timeIsFrozen = false;
-
 	private static long lastReturnedProcessingTime = Long.MIN_VALUE;
 
 	private static final Object lock = new Object();
 
+	@Override
 	@GuardedBy("lock")
-	static long freeze() {
+	public long currentTimestamp() {
 		synchronized (lock) {
-			if (!timeIsFrozen || lastReturnedProcessingTime == Long.MIN_VALUE) {
-				timeIsFrozen = true;
-				return getCurrentTimestamp();
-			} else {
+			if (lastReturnedProcessingTime != Long.MIN_VALUE) {
 				return lastReturnedProcessingTime;
 			}
+			return getCurrentTimestamp();
 		}
 	}
 
-	@Override
-	@GuardedBy("lock")
-	public long currentTimestamp() {
+	static <T, E extends Throwable> T doWithFrozenTime(FunctionWithException<Long, T, E> action) throws E {
 		synchronized (lock) {
-			if (timeIsFrozen && lastReturnedProcessingTime != Long.MIN_VALUE) {
-				return lastReturnedProcessingTime;
-			}
-			return getCurrentTimestamp();
+			return action.apply(getCurrentTimestamp());
 
 Review comment:
   I think we still need to freeze and unfreeze time before and after this call asserting `timestampAfterUpdate == timestampBeforeUpdate` but now in a thread safe manner. Otherwise, this class does nothing and we again will have the problems with time discrepancies during verification which was the reason why we introduced the time freezing before. Looks like we do not really need to change the existing code too much, only introduce `doWithFrozenTime` and freeze/unfreeze methods can be private now w/o locking internally.
   
   Also, `doWithFrozenTime` can be annotated with `@GuardedBy("lock")`.
   
   Sorry, I guess my comment on Jira did not have the full code.

----------------------------------------------------------------
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


With regards,
Apache Git Services