You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zjureel <gi...@git.apache.org> on 2017/07/07 06:56:11 UTC
[GitHub] flink pull request #4278: [FLINK-7099] Replace usages of deprecated JOB_MANA...
GitHub user zjureel opened a pull request:
https://github.com/apache/flink/pull/4278
[FLINK-7099] Replace usages of deprecated JOB_MANAGER_IPC_PORT_KEY and JOB_MANAGER_IPC_ADDRESS_KEY
…d JOB_MANAGER_IPC_ADDRESS_KEY
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [ ] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [ ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [ ] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zjureel/flink FLINK-7099
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4278.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 #4278
----
commit 9abf61c22eb88d953082095ec8ea03ef7851f4ac
Author: zjureel <zj...@gmail.com>
Date: 2017-07-07T06:24:31Z
[FLINK-7099] Replace usages of deprecated JOB_MANAGER_IPC_PORT_KEY and JOB_MANAGER_IPC_ADDRESS_KEY
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4278: [FLINK-7099] Replace usages of deprecated JOB_MANA...
Posted by zjureel <gi...@git.apache.org>.
Github user zjureel commented on a diff in the pull request:
https://github.com/apache/flink/pull/4278#discussion_r126383468
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java ---
@@ -185,8 +185,8 @@ public static WebMonitor startWebMonitorIfConfigured(
// this ensures correct values are present in the web frontend
final Address address = AkkaUtils.getAddress(actorSystem);
- config.setString(ConfigConstants.JOB_MANAGER_IPC_ADDRESS_KEY, address.host().get());
- config.setString(ConfigConstants.JOB_MANAGER_IPC_PORT_KEY, address.port().get().toString());
+ config.setString(JobManagerOptions.ADDRESS, address.host().get());
+ config.setInteger(JobManagerOptions.PORT, Integer.parseInt(address.port().get().toString()));
--- End diff --
Okay, thank you for your reply :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4278: [FLINK-7099] Replace usages of deprecated JOB_MANA...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4278#discussion_r126346815
--- Diff: flink-contrib/flink-storm/src/main/java/org/apache/flink/storm/api/FlinkSubmitter.java ---
@@ -91,12 +91,11 @@ public static void submitTopology(final String name, final Map stormConf, final
final Configuration flinkConfig = GlobalConfiguration.loadConfiguration();
if (!stormConf.containsKey(Config.NIMBUS_HOST)) {
stormConf.put(Config.NIMBUS_HOST,
- flinkConfig.getString(ConfigConstants.JOB_MANAGER_IPC_ADDRESS_KEY, "localhost"));
+ flinkConfig.getString(JobManagerOptions.ADDRESS, "localhost"));
}
if (!stormConf.containsKey(Config.NIMBUS_THRIFT_PORT)) {
stormConf.put(Config.NIMBUS_THRIFT_PORT,
- new Integer(flinkConfig.getInteger(ConfigConstants.JOB_MANAGER_IPC_PORT_KEY,
--- End diff --
sounds good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4278: [FLINK-7099] Replace usages of deprecated JOB_MANA...
Posted by zjureel <gi...@git.apache.org>.
Github user zjureel commented on a diff in the pull request:
https://github.com/apache/flink/pull/4278#discussion_r126366568
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java ---
@@ -185,8 +185,8 @@ public static WebMonitor startWebMonitorIfConfigured(
// this ensures correct values are present in the web frontend
final Address address = AkkaUtils.getAddress(actorSystem);
- config.setString(ConfigConstants.JOB_MANAGER_IPC_ADDRESS_KEY, address.host().get());
- config.setString(ConfigConstants.JOB_MANAGER_IPC_PORT_KEY, address.port().get().toString());
+ config.setString(JobManagerOptions.ADDRESS, address.host().get());
+ config.setInteger(JobManagerOptions.PORT, Integer.parseInt(address.port().get().toString()));
--- End diff --
@zentol The returned value of `address.port().get()` in java is `Object` and I found it can't be used as `config.setInteger(JobManagerOptions.PORT, address.port().get()));` directly. Do you mean that we should use `config.setInteger(JobManagerOptions.PORT, (Integer)address.port().get()));` instead? Hoping to hear your suggestion, thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4278: [FLINK-7099] Replace usages of deprecated JOB_MANA...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4278#discussion_r126094060
--- Diff: flink-contrib/flink-storm/src/main/java/org/apache/flink/storm/api/FlinkSubmitter.java ---
@@ -91,12 +91,11 @@ public static void submitTopology(final String name, final Map stormConf, final
final Configuration flinkConfig = GlobalConfiguration.loadConfiguration();
if (!stormConf.containsKey(Config.NIMBUS_HOST)) {
stormConf.put(Config.NIMBUS_HOST,
- flinkConfig.getString(ConfigConstants.JOB_MANAGER_IPC_ADDRESS_KEY, "localhost"));
+ flinkConfig.getString(JobManagerOptions.ADDRESS, "localhost"));
}
if (!stormConf.containsKey(Config.NIMBUS_THRIFT_PORT)) {
stormConf.put(Config.NIMBUS_THRIFT_PORT,
- new Integer(flinkConfig.getInteger(ConfigConstants.JOB_MANAGER_IPC_PORT_KEY,
--- End diff --
missing override with 6123.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4278: [FLINK-7099] Replace usages of deprecated JOB_MANA...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4278
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4278: [FLINK-7099] Replace usages of deprecated JOB_MANA...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4278#discussion_r126381441
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java ---
@@ -185,8 +185,8 @@ public static WebMonitor startWebMonitorIfConfigured(
// this ensures correct values are present in the web frontend
final Address address = AkkaUtils.getAddress(actorSystem);
- config.setString(ConfigConstants.JOB_MANAGER_IPC_ADDRESS_KEY, address.host().get());
- config.setString(ConfigConstants.JOB_MANAGER_IPC_PORT_KEY, address.port().get().toString());
+ config.setString(JobManagerOptions.ADDRESS, address.host().get());
+ config.setInteger(JobManagerOptions.PORT, Integer.parseInt(address.port().get().toString()));
--- End diff --
I don't think we can cast it to Integer since it is a scala Int actually...let's keep it as it is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4278: [FLINK-7099] Replace usages of deprecated JOB_MANAGER_IPC...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4278
alright, I will merge this now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4278: [FLINK-7099] Replace usages of deprecated JOB_MANA...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4278#discussion_r126094196
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java ---
@@ -185,8 +185,8 @@ public static WebMonitor startWebMonitorIfConfigured(
// this ensures correct values are present in the web frontend
final Address address = AkkaUtils.getAddress(actorSystem);
- config.setString(ConfigConstants.JOB_MANAGER_IPC_ADDRESS_KEY, address.host().get());
- config.setString(ConfigConstants.JOB_MANAGER_IPC_PORT_KEY, address.port().get().toString());
+ config.setString(JobManagerOptions.ADDRESS, address.host().get());
+ config.setInteger(JobManagerOptions.PORT, Integer.parseInt(address.port().get().toString()));
--- End diff --
skip conversion using `toString()`, then we don't need to parse it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4278: [FLINK-7099] Replace usages of deprecated JOB_MANA...
Posted by zjureel <gi...@git.apache.org>.
Github user zjureel commented on a diff in the pull request:
https://github.com/apache/flink/pull/4278#discussion_r126343858
--- Diff: flink-contrib/flink-storm/src/main/java/org/apache/flink/storm/api/FlinkSubmitter.java ---
@@ -91,12 +91,11 @@ public static void submitTopology(final String name, final Map stormConf, final
final Configuration flinkConfig = GlobalConfiguration.loadConfiguration();
if (!stormConf.containsKey(Config.NIMBUS_HOST)) {
stormConf.put(Config.NIMBUS_HOST,
- flinkConfig.getString(ConfigConstants.JOB_MANAGER_IPC_ADDRESS_KEY, "localhost"));
+ flinkConfig.getString(JobManagerOptions.ADDRESS, "localhost"));
}
if (!stormConf.containsKey(Config.NIMBUS_THRIFT_PORT)) {
stormConf.put(Config.NIMBUS_THRIFT_PORT,
- new Integer(flinkConfig.getInteger(ConfigConstants.JOB_MANAGER_IPC_PORT_KEY,
--- End diff --
@zentol Thank you for your review. The default value of `JobManagerOptions.PORT` is 6123, I think maybe we use `flinkConfig.getInteger(JobManagerOptions.PORT)` here instead of `flinkConfig.getInteger(JobManagerOptions.PORT, 6123)` is better. What do you think? Thanks :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---