Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1097
---
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 rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158939653
Thank you for addressing the issues so timely.
I'll merge the change now, thank you!
---
If your project is set up for it, you can reply to this email and
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158761960
the build failure is due to a test in streaming, so most likely not related.
the test failure has been observed before and is captured in issue
FLINK-2348
---
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158765143
I've tested the PR again.
I found a minor issue with the alternative arguments.
input: `--input abc --p 12`,
code:
```java
RequiredParameters rq =
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158766866
yep, based on the discussion above with fabian the default value is set
only on the long key.
I'll go ahead and set it on the short key as well, if nothing
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158784004
ok. done.
---
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
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158775559
Oh, I see. I have to admit I didn't closely follow the discussion with
Fabian.
Would be nice if you could change the behavior.
---
If your project is set up
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158340342
I'm wondering whether you are trying to run the examples I'm posting here:
```java
RequiredParameters rq = new RequiredParameters();
rq.add("input");
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158345168
Oha. that is embarrassing, I thought I had it covered. I'll look into it.
Sorry for the mess.
---
If your project is set up for it, you can reply to this email and
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158335983
Yes, in IntelliJ the button right to the Debug button will generate a test
coverage report.
---
If your project is set up for it, you can reply to this email and have
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158329310
Cool. Pushed my latest changes which fix the bug and add more test cases.
Btw., is there any way to see a test coverage report?
---
If your project is
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158321550
No, I think once that's resolved, the PR is good to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158164663
Oha. yes. Found the problem causing the exception, will fix it and make the
tests more specific as to which exceptions they need to catch.
While I am at it,
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-158121645
I think the test cases are not good enough.
I tried it out with this code again:
```java
RequiredParameters rq = new RequiredParameters();
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45172895
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/Option.java ---
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-157648501
I'll address the comments and rework the getHelp method to produce more
useful output (and cover that with more tests).
---
If your project is set up for it, you
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45173702
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java
---
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45178470
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/Option.java ---
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45173470
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java
---
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45173590
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/Option.java ---
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-157660361
Thank you for reacting so quickly to my feedback.
I'm sorry for the delays during the PR review.
---
If your project is set up for it, you can reply to this email
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-157648517
thanks for the feedback.
---
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
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45173689
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/OptionType.java ---
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-157645907
Thank you for reviewing the pull request while I was away @fhueske.
I've just tried it out with this code:
```java
RequiredParameters rp = new
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45172709
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java
---
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45178597
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/OptionType.java ---
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45173067
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java
---
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r45173029
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/OptionType.java ---
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-157713303
Sure, no problem.
One comment concerning the comment:
The `RequiredParametersException` could list the missing arguments.
At the moment the
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-157717077
I think it should not be too hard to collect all the issues in a list and
then throw the exception in the end.
What do you mean by the optional list? Only listing
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-157719075
optional in the sense of, if the list is passed, then the missing arguments
are added to the help text in an extra paragraph at the end.
realized probably via
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-157719256
Okay, I think its a good idea.
---
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
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-157722421
cool. will update the PR today.
---
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
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-155437914
Hi @rmetzger, do you want to give it another look and check if it does what
you intended with FLINK-2017?
---
If your project is set up for it, you can reply to this
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-152135619
I think it's fine to have an `add(Option)` method in addition to
`add(String)`. There might be cases where you want to create the Options first
and add them later on.
---
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-152144589
Ok, should I rebase on the current master and squash the commits into one
before it will be merged?
---
If your project is set up for it, you can reply to this
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-152147439
Yes, that would be nice. 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
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-151804263
Yes, that's right.
I'll address the comments tonight. If the `add(Option)` issue is blocking
the merge, we can just drop the function it, if you want.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r43236711
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java
---
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r43236812
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java
---
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-151791276
Thanks for the update!
Except for my comments, the only remaining question is whether to offer the
`add(Option)` method or not, right?
---
If your project is set
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r43241508
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java
---
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r43241539
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java
---
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r43101847
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-151439931
I do see your point about the uselessness of only `check`, so I'll go ahead
and implement `applyTo`.
I'll update the PR tonight.
---
If your project is
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r43100940
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-151441146
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
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-151142957
Going through FLINK-2017, I interpreted the semantics of `check` and
`checkAndPopulate` as follows:
- `check` verifies that all required parameters are provided and
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r42993834
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-150490748
I think that might just boil down to the question whether or not there is a
common enough use case for one without the other.
If so, we could change it to one
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r42650529
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-149961803
Looks like good progress! As pointed out in one of my comments, I think we
should make the use a bit more intuitive. Having two methods which are named so
similar but
Github user chiwanpark commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r40297666
--- Diff:
flink-java/src/test/java/org/apache/flink/api/java/utils/RequiredParameterTest.java
---
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-142857451
I'll soon review this pull request. Sorry for the delay.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-142684451
The test failure seems unrelated to my changes. It's connected to the
YARNSessionFIFOITCase test.
FLINK-2700 has been opened a couple of days ago with the
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39933413
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39493200
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39492715
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39493290
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-140343995
Thanks for the PR @mliesenberg!
Can you add the checks (and tests to verify the checks are working) that I
mentioned in my comments?
The original JIRA issue also
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39492448
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/Option.java ---
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-140347951
If you add a type field (or enum) to the option, you can check if you can
cast the string into the requested type. Since we would only support a fixed
set of types (the
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39495024
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39497481
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39495292
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-140347682
@fhueske Yes, I will add the checks.
Regarding the type checks: Does that refer to the interaction with the
ParameterTool class? I thought about that as
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-140348478
Ok, I will add that.
---
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
Github user mliesenberg commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39496043
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39496768
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1097#discussion_r39068191
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameter.java
---
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-138229357
Thanks for the PR! I am sure that the mentioned test failure is unrelated
to your changes. If you like, you can create a new issue with the label
test-instability for the
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-138243384
OK, cool. I have created a separate ticket.
https://issues.apache.org/jira/browse/FLINK-2628
---
If your project is set up for it, you can reply to this email and
Github user mliesenberg commented on the pull request:
https://github.com/apache/flink/pull/1097#issuecomment-138189037
One of the builds failed because of a test failure in:
CoStreamCheckpointingITCase>StreamFaultToleranceTestBase.runCheckpointedProgram:105->postSubmit:126
GitHub user mliesenberg opened a pull request:
https://github.com/apache/flink/pull/1097
[FLINK-2017] Add predefined required parameters to ParameterTool
- adds Option class to represent required parameter
- adds RequiredParameter class to interact with ParameterTool to check
74 matches
Mail list logo
