Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1046
---
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
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-141851757
Hi @fhueske , i will update the PR to use ExecutionConfig. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-141550369
Hi @ffbin,
not sure if you followed the discussion on the mailing list, but we
discussed to use the ExecutionConfig instead of the JobConfig. The reason is
that
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-140291773
As per discussion on the dev list, the `ExecuionConfig` has the
`GlobalJobParameters`, which are useful if one type of config is used across
all operators.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-140301378
Storm only supports one global configuration that is shared over all
spout/bolts. So `GlobalJobParameter` will work just fine.
---
If your project is set up for it, you
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r39453328
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/runtime/tasks/StreamingRuntimeContext.java
---
@@
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r39452503
--- Diff:
flink-core/src/main/java/org/apache/flink/configuration/Configuration.java ---
@@ -418,6 +418,17 @@ public void setBytes(String key, byte[] bytes)
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r39453004
--- Diff:
flink-core/src/main/java/org/apache/flink/configuration/Configuration.java ---
@@ -418,6 +418,17 @@ public void setBytes(String key, byte[] bytes) {
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r39450885
--- Diff: docs/apis/storm_compatibility.md ---
@@ -169,6 +169,13 @@ The input type is `Tuple1` and
`Fields("sentence")` specify that `input.
See
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r39450937
--- Diff: docs/apis/storm_compatibility.md ---
@@ -169,6 +169,13 @@ The input type is `Tuple1` and
`Fields("sentence")` specify that `input.
See
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-140222747
Actually, I think going through the `TaskConfig` as proposed by @mjsax is
the cleaner way. Going through the system-internal `JobConfiguration` and
exposing it to user
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-140224226
It is not clear (at least to me) how to do this. The API does not offer an
(obvious) way to set a configuration... (or I just don't get it).
`StreamExecutionEnvironment`
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-139959540
@StephanEwen @rmetzger Can you have a look at it if it can be merged? I am
also work on storm task hooks and it depend on this PR. Thank you very much!
---
If your
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-138865484
@mjsax Thanks. I have finish the change about all comments.
@StephanEwen @rmetzger Can you have a look at it if it can be merged?
Thank you very much!
---
If your
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-138470052
@mjsax @StephanEwen @rmetzger I have finish the change about all comments
and update documentation. Can you have a look at it if it can be merged? Thank
you very much!
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38914538
--- Diff:
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-138524518
I just had a few "cosmetic" comments. Otherwise it looks good to me to get
merged.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38913931
--- Diff: docs/apis/storm_compatibility.md ---
@@ -201,6 +201,26 @@ DataStream s2 =
splitStream.select("s2").transform(/* use Bolt f
See
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38913193
--- Diff: docs/apis/storm_compatibility.md ---
@@ -201,6 +201,26 @@ DataStream s2 =
splitStream.select("s2").transform(/* use Bolt f
See
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38914417
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38914633
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38913061
--- Diff: docs/apis/storm_compatibility.md ---
@@ -201,6 +201,26 @@ DataStream s2 =
splitStream.select("s2").transform(/* use Bolt f
See
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38914053
--- Diff: docs/apis/storm_compatibility.md ---
@@ -201,6 +201,26 @@ DataStream s2 =
splitStream.select("s2").transform(/* use Bolt f
See
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38844802
--- Diff:
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38844654
--- Diff:
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38844529
--- Diff:
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-138292133
@mjsax @StephanEwen @rmetzger I have finish the change about all comments.
Can you have a look at it if it can be merged? Thank you very much!
---
If your project is set
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-138267349
@rmetzger Thanks.I have use the classloader of `StormWrapperSetupHelper´
instead of 'map' and change the fashion of creating Strings.
---
If your project is set up for
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38860604
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38866736
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38865859
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38866357
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38867651
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38867892
--- Diff:
flink-contrib/flink-storm-compatibility/flink-storm-compatibility-examples/src/main/java/org/apache/flink/stormcompatibility/util/StormFileSpout.java
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38867760
--- Diff:
flink-contrib/flink-storm-compatibility/flink-storm-compatibility-examples/src/main/java/org/apache/flink/stormcompatibility/util/StormFileSpout.java
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38867724
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38867413
--- Diff:
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-138320697
Can you update documentation, too? For README.md just delete the line that
claims configuration is not supported. WebPage documentation should contain a
short paragraph
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-138051794
@mjsax @StephanEwen Thanks. I have finish the changes at the core classes.
when user use env.getConfig().setGlobalJobParameters(conf); to set storm
configuration, I
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-137748262
Concerning the changes at the core classes:
- The storm config key is an application specific key and not part of the
system configuration, therefore it should
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-137742282
I have just a quick look over it, and so far it like it. Two things are
open to be discussed. I not sure it the change to `ConfigConstants` in a good
choice. Would it be
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-137748448
The changes suggested above help with a cleaner separation between the
application (here storm compatibility) and the core code.
---
If your project is set up for
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38456958
--- Diff:
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-136636200
@mjsax @StephanEwen I have finish the code change.Can you give me some
comment? Thank you very much!
---
If your project is set up for it, you can reply to this email
Github user ffbin commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38382368
--- Diff:
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-136302889
Travis run on Linux. There is only a single ":" in the path there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-136301903
@mjsax Thanks.
I have a question:
In my windows machine, the textPath of ExclamationWithStormSpoutITCase is
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-136289777
This is the stack trace (occurs in 4/5 runs -- the other run failed before
due to unrelated test). It seems you broke something.
```
Tests run: 1, Failures: 1,
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-136304296
Oh, So not all test case can run successfully in windows?
I have change this and commit again.
---
If your project is set up for it, you can reply to this email and
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-136305354
This might be the case. I never tried it. And as far as I know, all
developers work on Linux or Mac, so this was never an issue.
---
If your project is set up for it,
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38312505
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38312609
--- Diff:
flink-contrib/flink-storm-compatibility/flink-storm-compatibility-examples/src/main/java/org/apache/flink/stormcompatibility/util/StormFileSpout.java
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38312152
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38312112
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38312165
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38312340
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38311967
--- Diff:
flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/wrappers/StormBoltWrapper.java
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38311822
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38312308
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r38312802
--- Diff:
flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java ---
@@ -275,7 +275,14 @@
* Path to Hadoop configuration
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-136241503
@mjsax @StephanEwen I have finish the code changes.
1.serialize Storm Config as a byte[] into the Flink configuration
2.extend ExclamationTopology such that the
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-135412789
Oh. you are right. Serialize the whole Map into a single byte[] is
better.Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-135396475
You can turn the storm config into a byte[] via the `InstantiationUtil`
class. That byte[] can be stored in the regular config.
We could also add a nested
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-135399187
I think the `byte[]`converting approach is the correct way to go. Storm
config keys must not be `String`, thus the specific prefix trick cannot be
applied.
---
If your
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-135403521
@ffbin: Can you try if you can simply put the serialized Storm Config as a
byte[] into the Flink configuration? You can the unpack it inside the storm
code, when
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-135403187
@StephanEwen Thansk.The key of storm config is object, so maybe the
confData(HashMapString, Object) of Configuration is not enough.
---
If your project is set up for it,
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-135085374
Putting a nested stormConf into the configuration seems just wrong,
sorry. Such a specific hack in a generic utility cannot yield maintainable code.
Why is
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37976623
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37976536
--- Diff:
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-135000384
Can anyone have a look at `Configuration.java`. Not sure if the changes are
ok.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-135006223
@ffbin Can you extend `ExclamationTopology` such that the number of added
`! `in `ExclamationBolt` and `ExclamationWithStormSpout.ExclamationMap` is
configurable and
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37746515
--- Diff:
flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/api/FlinkLocalCluster.java
---
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37745589
--- Diff:
Github user ffbin commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37745377
--- Diff:
flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/api/FlinkLocalCluster.java
---
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-134198533
@mjsax .The reason why i can not see Travis details is that(from reply
mail):
The problem is that our CDN is currently blocked in mainland China. I'm
talking to our
Github user ffbin commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37746734
--- Diff:
Github user ffbin commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-134184818
@mjsax Thank you very much.I miss the change in FlinkClient.I will fix it
and test via bin/start-local.sh.In china, now we can not see the CI details and
it is hard to
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-134190885
It fails in two test. You should actually see it, if you execute test
locally. You should run test each time before you open/update an PR (at least
for the module you did
Github user ffbin commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37744576
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37745701
--- Diff:
flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/api/FlinkLocalCluster.java
---
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37746137
--- Diff:
flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/api/FlinkLocalCluster.java
---
Github user ffbin commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37745758
--- Diff:
Github user ffbin commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37746694
--- Diff:
GitHub user ffbin opened a pull request:
https://github.com/apache/flink/pull/1046
[FLINK-2525]Add configuration support in Storm-compatibility
- enable config can used in Spouts.open() and Bout.prepare().
Example like this:
public static void main(final String[] args)
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37741197
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37741352
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37741303
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37741464
--- Diff:
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/flink/pull/1046#discussion_r37741748
--- Diff:
flink-contrib/flink-storm-compatibility/flink-storm-compatibility-core/src/main/java/org/apache/flink/stormcompatibility/api/FlinkLocalCluster.java
---
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-134162565
I don't see changes in `FlinkClient`. Only in `FlinkLocalCluster`. Did you
test by starting Flink via `bin/start-local.sh` (it would be even better to
test in a real
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1046#issuecomment-134162997
Travis fails because you broke something...
---
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
91 matches
Mail list logo
