You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by ffbin <gi...@git.apache.org> on 2015/08/12 04:39:24 UTC
[GitHub] flink pull request: [FLINK-2512]Add client.close() before throw Ru...
GitHub user ffbin opened a pull request:
https://github.com/apache/flink/pull/1009
[FLINK-2512]Add client.close() before throw RuntimeException
In line 129, it close client in finally{} before throw exception.
But in line 105, it throw exception without close client.
So I think it is better to close client before throw RuntimeException.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ffbin/flink FLINK-2512
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1009.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 #1009
----
commit 7510fdb6a8a082d8be85ba7968e9e2760edf2af0
Author: ffbin <86...@qq.com>
Date: 2015-08-12T02:06:21Z
[FLINK-2512]Add client.close() before throw RuntimeException
----
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-131146003
Looks good, will merge this...
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-130363877
Since ``getTopologyJobId`` can also throw exception, we could just move up the try-catch to include the call to that method and rely on finally to close the client.
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-131288659
+1
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-130205016
I think the best thing would be to just move the `try` up a little.
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by ffbin <gi...@git.apache.org>.
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-130509029
@uce @hsaputra Thanks. I have move the try up and rely on finally to close the client.
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by ffbin <gi...@git.apache.org>.
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-131287408
@hsaputra @StephanEwen Thanks. I have fix my code error.
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/flink/pull/1009#discussion_r37087438
--- Diff: flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/api/FlinkSubmitter.java ---
@@ -99,23 +99,24 @@ public static void submitTopology(final String name, final Map stormConf, final
final String serConf = JSONValue.toJSONString(stormConf);
- final FlinkClient client = FlinkClient.getConfiguredClient(stormConf);
- if (client.getTopologyJobId(name) != null) {
- throw new RuntimeException("Topology with name `" + name + "` already exists on cluster");
- }
- String localJar = System.getProperty("storm.jar");
- if (localJar == null) {
- try {
- for (final File file : ((ContextEnvironment) ExecutionEnvironment.getExecutionEnvironment())
- .getJars()) {
- // TODO verify that there is onnly one jar
- localJar = file.getAbsolutePath();
+ try {
+ final FlinkClient client = FlinkClient.getConfiguredClient(stormConf);
--- End diff --
Shouldn't declaration of ``client`` be outside of the try clause?
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-131148157
All tests are failing though, not sure if bc this patch
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-131163742
You are right, CI caught it. From my skimming over, the code looked good, but it does not compile...
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1009
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-130323420
If you move the try up, you can certainly remove the manual close. Regarding the check in 103, it really depends on whether the Strom compat layer depends on having only a single job per client. Therefore I would keep it in and let it throw the RuntimeException as before. The finally block will then make sure that the client is closed.
---
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: [FLINK-2512]Add client.close() before throw Ru...
Posted by ffbin <gi...@git.apache.org>.
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1009#issuecomment-130233136
@uce Thanks. What about remove "if(client.getTopologyJobId(name) != null) {...}" in line 103, because submitTopologyWithOpts() has check it at the head of function and will throw AlreadyAliveException.Then in finally{} close client.
---
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.
---