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

update several deprecated configuration options

    Details

      Description

      1. We should use 'containerized.heap-cutoff-ratio' and 'containerized.heap-cutoff-min' instead of deprecated yarn-specific options in configuration doc.

      2. In mesos mode, we still use deprecated naming convention of zookeeper - 'recovery.zookeeper.path.mesos-workers'. We should make it consistent with other zookeeper options by using 'high-availability.zookeeper.path.mesos-workers'.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user barcahead opened a pull request:

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

          FLINK-5712 [config] update several deprecated configuration options

          1. We should use 'containerized.heap-cutoff-ratio' and 'containerized.heap-cutoff-min' instead of deprecated yarn-specific options in configuration doc.
          2. In mesos mode, we still use deprecated naming convention of zookeeper - 'recovery.zookeeper.path.mesos-workers'. We should make it consistent with other zookeeper options by using 'high-availability.zookeeper.path.mesos-workers'.

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

          $ git pull https://github.com/barcahead/flink flink5712

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

          https://github.com/apache/flink/pull/3267.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 #3267


          commit 6d85f2d4ca59c6adf211830b9b476e6f2ff24aa6
          Author: fengyelei <fengyel@gmail.com>
          Date: 2017-02-04T09:18:06Z

          FLINK-5712 [config] update several deprecated configuration options


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user barcahead opened a pull request: https://github.com/apache/flink/pull/3267 FLINK-5712 [config] update several deprecated configuration options 1. We should use 'containerized.heap-cutoff-ratio' and 'containerized.heap-cutoff-min' instead of deprecated yarn-specific options in configuration doc. 2. In mesos mode, we still use deprecated naming convention of zookeeper - 'recovery.zookeeper.path.mesos-workers'. We should make it consistent with other zookeeper options by using 'high-availability.zookeeper.path.mesos-workers'. You can merge this pull request into a Git repository by running: $ git pull https://github.com/barcahead/flink flink5712 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3267.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 #3267 commit 6d85f2d4ca59c6adf211830b9b476e6f2ff24aa6 Author: fengyelei <fengyel@gmail.com> Date: 2017-02-04T09:18:06Z FLINK-5712 [config] update several deprecated configuration options
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3267#discussion_r100556946

          — Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/services/MesosServicesUtils.java —
          @@ -40,9 +41,11 @@ public static MesosServices createMesosServices(Configuration configuration) thr
          return new StandaloneMesosServices();

          case ZOOKEEPER:

          • final String zkMesosRootPath = configuration.getString(
            + final String zkMesosRootPath = ConfigurationUtil.getStringWithDeprecatedKeys(
            + configuration,
            ConfigConstants.HA_ZOOKEEPER_MESOS_WORKERS_PATH,
          • ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH);
            + ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH,
            + ConfigConstants.ZOOKEEPER_MESOS_WORKERS_PATH);
              • End diff –

          I think it's better if we replace this directly by a `ConfigOption`. There you can also define deprecated keys. This is the encouraged way to read configuration values. Take a look at `HighAvailabilityOptions` to see how it is used.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on a diff in the pull request: https://github.com/apache/flink/pull/3267#discussion_r100556946 — Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/services/MesosServicesUtils.java — @@ -40,9 +41,11 @@ public static MesosServices createMesosServices(Configuration configuration) thr return new StandaloneMesosServices(); case ZOOKEEPER: final String zkMesosRootPath = configuration.getString( + final String zkMesosRootPath = ConfigurationUtil.getStringWithDeprecatedKeys( + configuration, ConfigConstants.HA_ZOOKEEPER_MESOS_WORKERS_PATH, ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH); + ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH, + ConfigConstants.ZOOKEEPER_MESOS_WORKERS_PATH); End diff – I think it's better if we replace this directly by a `ConfigOption`. There you can also define deprecated keys. This is the encouraged way to read configuration values. Take a look at `HighAvailabilityOptions` to see how it is used.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user barcahead commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3267#discussion_r100660350

          — Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/services/MesosServicesUtils.java —
          @@ -40,9 +41,11 @@ public static MesosServices createMesosServices(Configuration configuration) thr
          return new StandaloneMesosServices();

          case ZOOKEEPER:

          • final String zkMesosRootPath = configuration.getString(
            + final String zkMesosRootPath = ConfigurationUtil.getStringWithDeprecatedKeys(
            + configuration,
            ConfigConstants.HA_ZOOKEEPER_MESOS_WORKERS_PATH,
          • ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH);
            + ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH,
            + ConfigConstants.ZOOKEEPER_MESOS_WORKERS_PATH);
              • End diff –

          Thanks for the review.

          I looked into `HighAvailabilityOptions` and found that it doesn't contain all the HA options, some options are still in `ConfigConstants`.
          It looks like this part is still in the middle of refactoring, right? I also heard some discussion from @greghogan about generating configuration document from `ConfigOption` automatically.
          Is there any work I can help with, like move options from `ConfigConstants` to corresponding `xxConfigOptions` files or the automatic work? If it is the right direction and there is some work I can do, I would say to have another PR for the work, if not I would just move `HA_ZOOKEEPER_MESOS_WORKERS_PATH` to `HighAvailabilityOptions` and finish this PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user barcahead commented on a diff in the pull request: https://github.com/apache/flink/pull/3267#discussion_r100660350 — Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/services/MesosServicesUtils.java — @@ -40,9 +41,11 @@ public static MesosServices createMesosServices(Configuration configuration) thr return new StandaloneMesosServices(); case ZOOKEEPER: final String zkMesosRootPath = configuration.getString( + final String zkMesosRootPath = ConfigurationUtil.getStringWithDeprecatedKeys( + configuration, ConfigConstants.HA_ZOOKEEPER_MESOS_WORKERS_PATH, ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH); + ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH, + ConfigConstants.ZOOKEEPER_MESOS_WORKERS_PATH); End diff – Thanks for the review. I looked into `HighAvailabilityOptions` and found that it doesn't contain all the HA options, some options are still in `ConfigConstants`. It looks like this part is still in the middle of refactoring, right? I also heard some discussion from @greghogan about generating configuration document from `ConfigOption` automatically. Is there any work I can help with, like move options from `ConfigConstants` to corresponding `xxConfigOptions` files or the automatic work? If it is the right direction and there is some work I can do, I would say to have another PR for the work, if not I would just move `HA_ZOOKEEPER_MESOS_WORKERS_PATH` to `HighAvailabilityOptions` and finish this PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3267#discussion_r100695134

          — Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/services/MesosServicesUtils.java —
          @@ -40,9 +41,11 @@ public static MesosServices createMesosServices(Configuration configuration) thr
          return new StandaloneMesosServices();

          case ZOOKEEPER:

          • final String zkMesosRootPath = configuration.getString(
            + final String zkMesosRootPath = ConfigurationUtil.getStringWithDeprecatedKeys(
            + configuration,
            ConfigConstants.HA_ZOOKEEPER_MESOS_WORKERS_PATH,
          • ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH);
            + ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH,
            + ConfigConstants.ZOOKEEPER_MESOS_WORKERS_PATH);
              • End diff –

          It's basically still ongoing work. If you like to help there, then you're very welcome. Best if you choose an option set, create a JIRA for it and then get cracking

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on a diff in the pull request: https://github.com/apache/flink/pull/3267#discussion_r100695134 — Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/services/MesosServicesUtils.java — @@ -40,9 +41,11 @@ public static MesosServices createMesosServices(Configuration configuration) thr return new StandaloneMesosServices(); case ZOOKEEPER: final String zkMesosRootPath = configuration.getString( + final String zkMesosRootPath = ConfigurationUtil.getStringWithDeprecatedKeys( + configuration, ConfigConstants.HA_ZOOKEEPER_MESOS_WORKERS_PATH, ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH); + ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH, + ConfigConstants.ZOOKEEPER_MESOS_WORKERS_PATH); End diff – It's basically still ongoing work. If you like to help there, then you're very welcome. Best if you choose an option set, create a JIRA for it and then get cracking
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

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

          Merging this PR. We decided to convert the missing HA options to `ConfigOptions` with a separate PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3267 Merging this PR. We decided to convert the missing HA options to `ConfigOptions` with a separate PR.
          Hide
          till.rohrmann Till Rohrmann added a comment -

          Fixed via 4f47ccdcbd3c794522d9bd5b60d11c016a71b5e7

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

          Github user asfgit closed the pull request at:

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

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

          Github user barcahead commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3267#discussion_r100710125

          — Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/services/MesosServicesUtils.java —
          @@ -40,9 +41,11 @@ public static MesosServices createMesosServices(Configuration configuration) thr
          return new StandaloneMesosServices();

          case ZOOKEEPER:

          • final String zkMesosRootPath = configuration.getString(
            + final String zkMesosRootPath = ConfigurationUtil.getStringWithDeprecatedKeys(
            + configuration,
            ConfigConstants.HA_ZOOKEEPER_MESOS_WORKERS_PATH,
          • ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH);
            + ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH,
            + ConfigConstants.ZOOKEEPER_MESOS_WORKERS_PATH);
              • End diff –

          Got it, thank you

          Show
          githubbot ASF GitHub Bot added a comment - Github user barcahead commented on a diff in the pull request: https://github.com/apache/flink/pull/3267#discussion_r100710125 — Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/services/MesosServicesUtils.java — @@ -40,9 +41,11 @@ public static MesosServices createMesosServices(Configuration configuration) thr return new StandaloneMesosServices(); case ZOOKEEPER: final String zkMesosRootPath = configuration.getString( + final String zkMesosRootPath = ConfigurationUtil.getStringWithDeprecatedKeys( + configuration, ConfigConstants.HA_ZOOKEEPER_MESOS_WORKERS_PATH, ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH); + ConfigConstants.DEFAULT_ZOOKEEPER_MESOS_WORKERS_PATH, + ConfigConstants.ZOOKEEPER_MESOS_WORKERS_PATH); End diff – Got it, thank you

            People

            • Assignee:
              Unassigned
              Reporter:
              barcahead Yelei Feng
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development