Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6076

Let the HeartbeatManager interface extend HeartbeatTarget

    Details

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

      Description

      The HeartbeatManager interface sub classes HeartbeatManagerImpl and HeartbeatManagerSenderImpl all implement the HeartbeatTarget interface. I think we could let the HeartbeatManager interface extend the HeartbeatTarget interface. Furthermore, I think it would be nicer to instantiate the HeartbeatManagerImpls directly with a HeartbeatListener instance. That way we could get rid of the HeartbeatManager#start method.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user tillrohrmann opened a pull request:

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

          FLINK-6076 Refactor HeartbeatManager to extend HeartbeatTarget

          Let the `HeartbeatManager` extend the `HeartbeatTarget` interface, because all `HeartbeatManager` implementations implement the `HeartbeatTarget` as well. Furthermore, this PR removes the `HeartbeatManager#start` method and passes an `HeartbeatListener` instance directly to the `HeartbeatManagerImpl` and `HeartbeatManagerSenderImpl`.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/tillrohrmann/flink refactorHeartbeatManager

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3555.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3555


          commit f75c25c7a733c0249881aad62b0f9d99a8e95924
          Author: Till Rohrmann <trohrmann@apache.org>
          Date: 2017-03-13T15:52:56Z

          FLINK-6076 Refactor HeartbeatManager to extend HeartbeatTarget

          Remove start method from HeartbeatManager


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user tillrohrmann opened a pull request: https://github.com/apache/flink/pull/3555 FLINK-6076 Refactor HeartbeatManager to extend HeartbeatTarget Let the `HeartbeatManager` extend the `HeartbeatTarget` interface, because all `HeartbeatManager` implementations implement the `HeartbeatTarget` as well. Furthermore, this PR removes the `HeartbeatManager#start` method and passes an `HeartbeatListener` instance directly to the `HeartbeatManagerImpl` and `HeartbeatManagerSenderImpl`. You can merge this pull request into a Git repository by running: $ git pull https://github.com/tillrohrmann/flink refactorHeartbeatManager Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3555.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3555 commit f75c25c7a733c0249881aad62b0f9d99a8e95924 Author: Till Rohrmann <trohrmann@apache.org> Date: 2017-03-13T15:52:56Z FLINK-6076 Refactor HeartbeatManager to extend HeartbeatTarget Remove start method from HeartbeatManager
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhijiangW commented on the issue:

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

          @tillrohrmann , it is really good to pass the `HeartbeatListener` directly when construct the `HeartbeatManagerImpl`. When I realized the heartbeat logic between different components like TM and JM, it is a little difficult to control the process between threads in order to verify the timeout expectation in tests. Now the `HeartbeatListener` is exposed out of the world, the test can extend the `notifyHeartbeatTimeout` method to control the process more easily.

          And I also notice that you change the `sendHeartbeat` to `receiveHeartbeat`, have you considered to use `responseHeartbeat` instead which is constract with `requestHeartbeat`.

          BTW, what is your suggestions of my current pull requests related with this modification? Should I resubmit that after this improvement merging into master? Thank you!

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhijiangW commented on the issue: https://github.com/apache/flink/pull/3555 @tillrohrmann , it is really good to pass the `HeartbeatListener` directly when construct the `HeartbeatManagerImpl`. When I realized the heartbeat logic between different components like TM and JM, it is a little difficult to control the process between threads in order to verify the timeout expectation in tests. Now the `HeartbeatListener` is exposed out of the world, the test can extend the `notifyHeartbeatTimeout` method to control the process more easily. And I also notice that you change the `sendHeartbeat` to `receiveHeartbeat`, have you considered to use `responseHeartbeat` instead which is constract with `requestHeartbeat`. BTW, what is your suggestions of my current pull requests related with this modification? Should I resubmit that after this improvement merging into master? Thank you!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

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

          Thanks for the review @zhijiangW. Yes I think it is better to initialize as much as possible in the constructor instead of calling methods afterwards.

          I'm not sure whether `responseHeartbeat` is the best name since I like to start methods with a verb which describe what this method does. But I'm also not super excited about the current names. If you come up with better names, then let's change it then.

          I've already adapted your PR and will merge it later today. I think for the remaining PRs, it's best if you take a look at the changes and adapt the other PRs accordingly.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3555 Thanks for the review @zhijiangW. Yes I think it is better to initialize as much as possible in the constructor instead of calling methods afterwards. I'm not sure whether `responseHeartbeat` is the best name since I like to start methods with a verb which describe what this method does. But I'm also not super excited about the current names. If you come up with better names, then let's change it then. I've already adapted your PR and will merge it later today. I think for the remaining PRs, it's best if you take a look at the changes and adapt the other PRs accordingly.
          Hide
          till.rohrmann Till Rohrmann added a comment -

          Fixed via 8765f324108c6cf20569c271776594b314aa91b7

          Show
          till.rohrmann Till Rohrmann added a comment - Fixed via 8765f324108c6cf20569c271776594b314aa91b7
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3555

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development