Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5029
Looks good, thanks!
Merging to master and 1.4...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5041
Merging this...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5041
Very good change, and nice implementation!
+1 to merge this.
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5042
Looks good to me.
Do we know if there are any new known issues in shade "3.0.0"? The first
release of a new major version sometimes has a few regressions. Have you
checked
Github user StephanEwen closed the pull request at:
https://github.com/apache/flink/pull/4943
---
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4981#discussion_r150020494
--- Diff: flink-formats/flink-avro/pom.xml ---
@@ -185,17 +185,6 @@ under the License
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4973
Yes, that is how it should be.
+1
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4968
LGTM
+1 to merge
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4948
Thanks for understanding!
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4960
Crazy realization is that there are 91 pom.xml files in Flink.
Its a large project :-)
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4960
:yellow_heart:
Very nice!
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4943
I updated this PR with the following proposed solution:
1. Avro is always part of the user code space, and hence will be loaded
into the user code classloader. This solves multiple
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4952
I am wondering why we name explicitly S3 and not other parts that are not
tested in an automatic fashion, (like MapR specific Kafka Kerberos quirks).
The S3 code is at least tested on
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4948
Sorry, I am opposed to changing that.
Sometimes an author explicitly wants to append parts of the parent
exception message to make it easier to navigate exception stack traces. I think
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4954
+1
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4943
Thanks for the review, addressing the comments.
I found the problem with the backwards compatibility: It is the Avro
version upgrade.
Avro types generated with Avro 1.7.7 (Flink 1.3
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4943#discussion_r148977803
--- Diff:
flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/BackwardsCompatibleAvroSerializer.java
---
@@ -0,0 +1,215
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4943#discussion_r148977780
--- Diff:
flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/BackwardsCompatibleAvroSerializer.java
---
@@ -0,0 +1,215
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4943#discussion_r148977758
--- Diff:
flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/AvroSerializer.java
---
@@ -153,111 +146,215 @@ public T
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4943
[FLINK-6022] [avro] Use Avro to serialize Avro in flight and in State
## What is the purpose of the change
This changes Avro types to be serialized with a proper Avro serializer. The
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4942
Lets see if Travis agrees. That guy has opinions...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4931
I posted a followup to this pull request in #4942
---
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4942
[FLINK-7420] [avro] Move all Avro code to flink-avro (followup)
## What is the purpose of the change
This is an extension of #4931 which adds some cleanups and improvements.
Most
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4939#discussion_r148656504
--- Diff: flink-filesystems/flink-s3-fs-hadoop/pom.xml ---
@@ -182,6 +182,21 @@ under the License.
${project.version
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4939#discussion_r148656918
--- Diff:
flink-filesystems/flink-s3-fs-hadoop/src/test/java/org/apache/flink/fs/s3hadoop/HadoopS3FileSystemITCase.java
---
@@ -57,11 +62,52
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4938
[FLINK-7968] [core] Move DataOutputSerializer and DataInputDeserializer to
'flink-core'
## What is the purpose of the change
This moves the classes `DataInputDeseria
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4916
Thanks for the follow-up!
+1 to merge this
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4927
Very nice, thanks!
Merging this...
---
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148526210
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/jobmanager/scheduler/Scheduler.java
---
@@ -133,32 +133,33 @@ public void shutdown
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148526103
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/Execution.java
---
@@ -1065,6 +1177,46 @@ private void
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4931
Changes look good all in all.
Missing one last pass for "Replace GenericData.Array by dummy when reading
TypeSerializers"...
---
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148363944
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/Execution.java
---
@@ -1065,6 +1177,46 @@ private void
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148360937
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/Execution.java
---
@@ -126,9 +134,11 @@
/** A future that
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148360739
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/Execution.java
---
@@ -220,14 +233,54 @@ public long getGlobalModVersion
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148366967
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/jobmanager/scheduler/Scheduler.java
---
@@ -133,32 +133,33 @@ public void shutdown
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148367405
--- Diff: flink-runtime/src/test/resources/log4j-test.properties ---
@@ -16,7 +16,7 @@
# limitations under the License
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148364165
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/Execution.java
---
@@ -1065,6 +1177,46 @@ private void
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148366296
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ExecutionVertex.java
---
@@ -476,14 +482,13 @@ else if (numSources
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4926
+1 to merge this
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4926
Thanks @steveloughran!
I think this approach here should work for then, for now.
---
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4916#discussion_r148358215
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ExecutionGraph.java
---
@@ -878,113 +880,70 @@ private void scheduleEager
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4926#discussion_r148347804
--- Diff:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationMasterRunner.java
---
@@ -265,7 +266,8 @@ protected int runApplicationMaster
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4926#discussion_r148347764
--- Diff:
flink-filesystems/flink-hadoop-fs/src/main/java/org/apache/flink/runtime/util/HadoopUtils.java
---
@@ -44,7 +44,9 @@
public
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4927
@zentol I like the changes!
Last two points:
1. Does it make sense to add only the non relocated guava classes in
`flink-shaded-curator`? Meaning define a filter in the shading
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4926
@steveloughran if it is okay, maybe we could pick could brain here quickly?
What is the Yarn-ideomatic way to handle configurations and what are the
assumptions?
Specifically:
- Do
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4931
Doing a review now...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4927
Looks good!
I would suggest two followups:
1. We can probably drop `flink-shaded-curator` as well, because curator
shaded guava by itself in the currently used version (still needs
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4920
My first thought is that moving libraries around is maybe not the best
approach.
Also, this seems like it implements a mix between explicitly configuring
the Hadoop Classpath and implicitly
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4920
Can you describe that this does and how it works?
Only getting a rough idea from the shell scripts...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4794
The `masters` and `slaves` file probably does not need a license header
(although the rules of when you need one and when not are not very clear to me).
I think config files frequently
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4777
Looks good - still the open question whether we add dependency convergence
by default, and deactivate it in not yet done modules. That gives the completed
modules a "done" feeling a
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4914
Good fix, merging...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4913
Thanks, good catch!
Merging...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4912
Thanks, merging...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4876
But do you understand what I mean? Semantics of code in the main scope
should not be quirked to make assertions in tests shorter to write.
Equals/hashCode is usually not implemented on I
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4908
Thanks, looks good, +1 to merge
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4827
Merged in 8595dadb89a1276c6c7d0ed2e2fae396a5c1d222
---
Github user StephanEwen closed the pull request at:
https://github.com/apache/flink/pull/4827
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4903
Sounds fair, +1
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4844
Had an offline discussion with @tillrohrmann - rewriting this without
Mockito results in a similar amount of code with similar maintenance effort, so
seems to be okay in this case.
+1
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4827
Merging as soon as Travis gives a green light on the rebased branch...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4827
Thanks for the review, merging...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4876
A quick post-mortem comment here:
This adds a lot of `equals()` and `hashCode()` on classes where these are
ill-defined.
For example: `StreamWriterBase` defines `equals()` and
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4827#discussion_r147165565
--- Diff: flink-test-utils-parent/flink-test-utils/pom.xml ---
@@ -117,6 +124,41 @@ under the License.
true
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4844#discussion_r147162652
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
---
@@ -1270,6 +1272,42 @@ public void run
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4907#discussion_r147092074
--- Diff: flink-core/src/main/java/org/apache/flink/util/FileUtils.java ---
@@ -243,11 +245,19 @@ else if (directory.exists()) {
* @throws
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4907#discussion_r147091056
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/state/filesystem/FsStateBackendFactory.java
---
@@ -18,45 +18,41 @@
package
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4907#discussion_r147090655
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/state/filesystem/FsStateBackend.java
---
@@ -18,54 +18,92 @@
package
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4907#discussion_r147090207
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/state/filesystem/AbstractFileStateBackend.java
---
@@ -0,0 +1,206
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4907#discussion_r147089435
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/state/ConfigurableStateBackend.java
---
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4907#discussion_r147089325
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ExecutionGraphBuilder.java
---
@@ -229,29 +229,31 @@ public static
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4907#discussion_r147089150
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
---
@@ -116,6 +117,10 @@
* accessing this
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4907#discussion_r147088900
--- Diff:
flink-core/src/main/java/org/apache/flink/configuration/CheckpointingOptions.java
---
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4907#discussion_r147088825
--- Diff:
flink-core/src/main/java/org/apache/flink/configuration/CheckpointingOptions.java
---
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4827#discussion_r147088428
--- Diff: flink-runtime/pom.xml ---
@@ -427,17 +427,42 @@ under the License.
shade
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4827#discussion_r147088234
--- Diff: flink-test-utils-parent/flink-test-utils/pom.xml ---
@@ -117,6 +124,41 @@ under the License.
true
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4827#discussion_r147087742
--- Diff: flink-runtime/pom.xml ---
@@ -427,17 +427,42 @@ under the License.
shade
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4907
[FLINK-5823] [checkpoints] State Backends also handle Checkpoint Metadata
(part 1)
This is an incremental (first part) rebuild of #3522 on the latest master.
For ease of review, broken
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4905
Looks good, +1 to merge this
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4900
I see, okay, the status checking in `trigger()` together with the tight
locking contract with StreamTask fixes that.
My feeling is that this is a workaround to support another non-ideal
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4902
Looks good, +1 to merge this
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4903
Change looks good.
Is `50 ms` also akka's default value?
Out of curiosity, what triggered the need to introduce this option.
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4900
I think that `quiesce()` should be called before `close()` is called on the
operator, so that after `close()` no new timers can fire. That was the main
purpose of the original change, because
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4896
The diffs looks good, but what I cannot judge in a final manner is whether
some tests now get not executed any more (accidentally).
What would be good is to take the Travis output from
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4891
Looks good to me, +1
When we start introducing even more config options for class loading, it
makes sense to introduce a class to bundle those up (like resolve order, parent
first
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4836
+1 to merge this
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4836
Merging...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4847
Merged in 51e526f88e460c9c8b936368714af9f15daf7fa6
I think the Apache / Github integration will not automatically close this
PR (auto closing works only PRs
against the `master
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4847
Nice, thanks, merging this...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4854
Merging for master and 1.3...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4869
Looks good to me+1
@zentol What do you think about this?
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4877
Thanks for opening this pull request.
I think it is correct that `SourceFunction` is not required to implement
`Serializable`. But I think it does not cause any problems, as far as I
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4885
Good fixed, merging to `master` and `release-1.3`
---
Github user StephanEwen closed the pull request at:
https://github.com/apache/flink/pull/4892
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4892
Merged in 9b73a5a49812c206bf8c94e5e4c6f616b6d858b7
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4891
Concerning loggers: I think for logging frameworks, they should rarely
bleed through, but if you instantiate multiple ones, you get problems. It is
more serious for the logging backend (where
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4891#discussion_r146573737
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobManagerServices.java
---
@@ -116,8 +116,15 @@ public static
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4891#discussion_r146573602
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/FlinkUserCodeClassLoaders.java
---
@@ -31,24 +31,23
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4891
Good fix, with minor comments.
I think we want to have even more default parent-first classes, because
they leak through the public API.
- `org.slf4j`
- `javax.annotation`
---
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4891#discussion_r146336888
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobManagerServices.java
---
@@ -116,8 +116,15 @@ public static
501 - 600 of 4278 matches
Mail list logo
