Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3.0
    • Fix Version/s: 1.3.0
    • Labels:
      None

      Description

      Currently, the method RpcService.scheduleRunnable does not return a control instance for the scheduled runnable. I think it would be good to return a ScheduledFuture with which one can cancel the scheduled runnable after it has been scheduled, e.g. a timeout registration which became obsolete. This API is also more in line with the ScheduledExecutorService where one also receives a ScheduledFuture after scheduling a runnable.

        Issue Links

          Activity

          Hide
          zjwang zhijiang added a comment - - edited

          This fix is useful and the HeartbeatManagerSenderImpl can get benefit from it. After this is done, it is better for HeartbeatManagerSenderImpl to use the RpcService.scheduleRunnable to schedule heartbeat by interval time.

          Show
          zjwang zhijiang added a comment - - edited This fix is useful and the HeartbeatManagerSenderImpl can get benefit from it. After this is done, it is better for HeartbeatManagerSenderImpl to use the RpcService.scheduleRunnable to schedule heartbeat by interval time.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

          https://github.com/apache/flink/pull/3311

          Thanks for the review @StephanEwen.

          You mean something like the `Executor` interface with the additional methods for the scheduled execution. Differently said, removing the `Service` methods like `shutdown`, `shutdownNow` and `awaitTermination` from the interface?

          That is a good idea. If we should stumble across the problem that a component needs an actual `ScheduledExecutorService` implementation, then we can still have a wrapper which throws an `UnsupportedOperationException` when calling the respective methods. I will adapt the PR in that way.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3311 Thanks for the review @StephanEwen. You mean something like the `Executor` interface with the additional methods for the scheduled execution. Differently said, removing the `Service` methods like `shutdown`, `shutdownNow` and `awaitTermination` from the interface? That is a good idea. If we should stumble across the problem that a component needs an actual `ScheduledExecutorService` implementation, then we can still have a wrapper which throws an `UnsupportedOperationException` when calling the respective methods. I will adapt the PR in that way.
          Hide
          till.rohrmann Till Rohrmann added a comment -

          wangzj, yes you're right. We can change this in the heartbeat PRs once the PR has been merged.

          Show
          till.rohrmann Till Rohrmann added a comment - wangzj , yes you're right. We can change this in the heartbeat PRs once the PR has been merged.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

          https://github.com/apache/flink/pull/3311

          I've introduce the `ScheduleExecutor` interface which extends the `Executor` interface with the `schedule` methods but not the service control methods of the `ScheduledExecutorService` interface.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3311 I've introduce the `ScheduleExecutor` interface which extends the `Executor` interface with the `schedule` methods but not the service control methods of the `ScheduledExecutorService` interface.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

          https://github.com/apache/flink/pull/3311

          Thanks for the review @StephanEwen. Merging this PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3311 Thanks for the review @StephanEwen. Merging this PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3311

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3311
          Hide
          till.rohrmann Till Rohrmann added a comment -

          Fixed via 0ba08b44477cd6eaade4e209230645148b303787

          Show
          till.rohrmann Till Rohrmann added a comment - Fixed via 0ba08b44477cd6eaade4e209230645148b303787

            People

            • Assignee:
              till.rohrmann Till Rohrmann
              Reporter:
              till.rohrmann Till Rohrmann
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development