Myasuka commented on a change in pull request #18931: URL: https://github.com/apache/flink/pull/18931#discussion_r817347164
########## File path: flink-dstl/flink-dstl-dfs/src/main/java/org/apache/flink/changelog/fs/RetryingExecutor.java ########## @@ -102,35 +129,48 @@ public void close() throws Exception { */ private final AtomicBoolean attemptCompleted = new AtomicBoolean(false); Review comment: The relationship of `actionCompleted` with `attemptCompleted` is really important within each `RetriableTask`. I think we can improve the readability via follow steps: 1. Only kept one private constructor and drop the previous public constructor, let the `attemptCompleted` to be initialized in the constructor: ~~~java private RetriableTask( int current, AtomicBoolean actionCompleted, RetriableAction runnable, RetryPolicy retryPolicy, ScheduledExecutorService blockingExecutor, Histogram attemptsPerTaskHistogram, ScheduledExecutorService scheduler, Consumer<Throwable> failureCallback, AtomicInteger activeAttempts) { this.current = current; this.runnable = runnable; this.failureCallback = failureCallback; this.retryPolicy = retryPolicy; this.blockingExecutor = blockingExecutor; this.actionCompleted = actionCompleted; this.attemptsPerTaskHistogram = attemptsPerTaskHistogram; this.scheduler = scheduler; this.activeAttempts = activeAttempts; this.attemptCompleted = new AtomicBoolean(false); } ~~~ 2. Introduce a static method to create the 1st RetriableTask: ~~~java private static RetriableTask createRetriableAttempts( RetriableAction runnable, RetryPolicy retryPolicy, ScheduledExecutorService blockingExecutor, Histogram attemptsPerTaskHistogram, ScheduledExecutorService scheduler, Consumer<Throwable> failureCallback) { return new RetriableTask( 1, new AtomicBoolean(false), runnable, retryPolicy, blockingExecutor, attemptsPerTaskHistogram, scheduler, failureCallback, new AtomicInteger(1)); } ~~~ 3. Rename the `next()` method to `nextAttempt()` ~~~ java private RetriableTask nextAttempt() { return new RetriableTask( current + 1, actionCompleted, runnable, retryPolicy, blockingExecutor, attemptsPerTaskHistogram, scheduler, failureCallback, activeAttempts); } ~~~ What do you think of my prosoals? -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org