Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4891#discussion_r146336120
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/FlinkUserCodeClassLoaders.java
---
@@ -31,24 +31,23
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4891#discussion_r146336544
--- 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_r146338877
--- Diff: docs/monitoring/debugging_classloading.md ---
@@ -57,22 +57,34 @@ YARN classloading differs between single job
deployments and sessions
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4892
[FLINK-7905] [build] Update encrypted Travis S3 access keys
This fixes the currently failing tests for S3 file systems by adding proper
encrypted access credentials.
The current
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4794
We already have a script, it is the RAT plugin:
https://github.com/apache/flink/blob/master/pom.xml#L957
You only need to make sure that these files are not excluded from the
check...
---
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4827
[FLINK-7840] [build] Shade netty in akka
## What is the purpose of the change
This change shade's Akka's dependency on Netty away.
**Note:** Akka itself cannot be s
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4787
@bowenli86 I hope you don't mind that I pushed back a bit. It's my job to
be careful about such things...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4524
Merging this...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4214
@skidder Do you want to follow up on this one? Otherwise, another
contributor might take this over.
Betting this fix into 1.4 would be great...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4822
Seems this is merged into master, please also merge this for 1.3.x
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4801
Eclipse Public License is not impossible, but tricky.
I am not a lawyer, but this is what I picked up over the year: EPL is weak
copyleft, meaning linking is okay, but modifying not
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4807
I think this is good now, +1
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4818
Thanks @steveloughran for the comments.
I am actually using Hadoop 2.8.1 here with AWS SDK 1.11.95.
The shaded artifacts are only a few MBs large, so this seems okay.
---
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4818#discussion_r144546384
--- Diff:
flink-filesystems/flink-s3-fs-hadoop/src/main/java/org/apache/flink/fs/s3hadoop/S3FileSystemFactory.java
---
@@ -0,0 +1,145
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4787
I think here is an interesting example of why I am often hesitant with
cleanup refactorings, unless there is a pressing need to clean up.
It is very hard to judge if the cleaned up
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4787
Actually, going back here. I would like to not merge this after all. The
reason being that in my test run, I found that this does not handle concurrent
deletes correctly after all:
https
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4816
Would be great to also have this in the 1.3 docs, but cherry-picking the
committ does not work...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4816
Thanks, merging this...
---
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4807#discussion_r144518821
--- Diff: flink-queryable-state/flink-queryable-state-java/pom.xml ---
@@ -110,8 +110,8 @@ under the License
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4807#discussion_r144518199
--- Diff: .travis.yml ---
@@ -61,31 +61,6 @@ matrix:
- TEST="misc"
- PROFILE="-Dhadoop.version=2.8.0"
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4807#discussion_r144519154
--- Diff:
flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskManagerComponentsStartupShutdownTest.java
---
@@ -202,8 +202,6
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4801
Thanks for this addition. Few comments:
- Please try to follow the common (though not enforced) code style when
it comes to empty lines between class declarations, fields, methods, etc
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4798
Probably good change for now.
I think in the long run, the TaskManager should give each Task a
sub-directory and make sure that sub directory is cleared whenever tasks
finish/cancel
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4787
Fair enough. Seems there are tests for the behavior already, so +1 to merge
this
Merging...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4794
Thanks, good addition.
Have you checked whether the shell scripts still work now, or whether they
get confused by the headers?
If all works, can you remove the exclusions from the
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4818
[FLINK-5706] [file systems] Add S3 file systems without Hadoop dependencies
## What is the purpose of the change
This adds two implementations of a file system that write to S3 so that
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4790
cc @aljoscha and @pnowojski
I think there is some work for Flink 1.4 to make the Kafka 0.10 sink a
regular sink function, so that the code that constructs the sink transformation
is not
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4787
Is there any problem with the current implementation?
The current implementation was carefully done to gracefully handle
concurrent removals and allow to pick whether to clean directories
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4749
I think this looks good.
Since it rename the mini cluster methods (meaning it does actually change
user facing pre-production code which should also be stable), I would only
merge this
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4751
@pnowojski I am wondering whether this is really necessary. This is not
quite a busy loop after all, because the `collect()` call emits to the Kafka
Producer and goes through buffering and I/O
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
This seems to be subsumed by #4445 - is that correct?
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4445
Agree with @KurtYoung.
Merging this...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4755
+1, merging this...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4756
Good from my side.
Since I am not very opinionated about docs/navigation, would be good to
have a second opinion.
@alpinegizmo what do you think?
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4754
Good catches, thanks.
Merging this...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4775
Thanks, will merge this without the added restart delay. If it is still
unstable, we can add that back.
---
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4775#discussion_r143254179
--- Diff:
flink-connectors/flink-connector-kafka-base/src/test/java/org/apache/flink/streaming/connectors/kafka/KafkaTestBase.java
---
@@ -121,10
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4777#discussion_r143253601
--- Diff: flink-core/pom.xml ---
@@ -77,6 +88,12 @@ under the License.
org.apache.commons
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4777#discussion_r143252870
--- Diff: flink-core/pom.xml ---
@@ -63,9 +63,20 @@ under the License.
com.esotericsoftware.kryo
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4777#discussion_r143253445
--- Diff: pom.xml ---
@@ -1181,7 +1204,7 @@ under the License.
org.apache.maven.plugins
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4783
Looks good, +1 to merge this
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4782
Thanks for fixing this, merging...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4773
Looks good to me!
---
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4774#discussion_r143248194
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/restart/FixedDelayRestartStrategy.java
---
@@ -82,7 +82,7 @@ public
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4774#discussion_r143246933
--- Diff:
flink-core/src/main/java/org/apache/flink/configuration/IllegalConfigurationException.java
---
@@ -42,6 +42,17 @@ public
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4781
[FLINK-7768] [core] Load File Systems via Java Service abstraction
**This is based on #4780 , so only the latest commit is relevant**
You can merge this pull request into a Git repository by
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4776
Thanks for the comments. Addressing them and merging them...
---
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4780
[FLINK-7766] [FLINK-7767] [file system sink] Cleanups in the streaming FS
sinks
**This build on top of #4776 - only the last two commits are relevant.**
## What is the purpose of the
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4776#discussion_r143142386
--- Diff: flink-core/src/main/java/org/apache/flink/core/fs/FileSystem.java
---
@@ -328,116 +360,54 @@ public static FileSystem getUnguardedFileSystem
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4776
@bowenli86 I can add a log statement.
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4776
@bowenli86 We need to fail lazily, because Flink should be able to always
work without MapR FS or HDFS being in the classpath.
With the change currently, you can start Flink without any
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4776#discussion_r142881593
--- Diff:
flink-core/src/main/java/org/apache/flink/core/fs/factories/HadoopFileSystemFactoryLoader.java
---
@@ -0,0 +1,67 @@
+/*
+ * Licensed
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/4776
[FLINK-7643] [core] Rework FileSystem loading to use factories
## What is the purpose of the change
This change reworks the loading and instantiation of File System objects
(including
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4771
Will address Zentol's comment and merge. This is blocking me...
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4616
+1 from my side to merge it like this (using boxed long, dropping the
`hasTimestamp()` method.
My reasoning is that this is consistent with `ProcessFunction` (like
@EronWright said), it
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4564
Did a side-by-side review with @aljoscha and directly fixed issues.
This is in very good shape from my side now.
+1 to merge this
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4445
Thanks!
I am currently trying to pinpoint what part of the code exactly suffers
most from the regression. If that is for example specific to the
microbenchmark, we can merge this
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4564
Thanks, this looks better to me!
I would suggest to not pass the configuration into the library cache
manager.
I think passing configuration objects into specialized / dedicated
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4525
I think `null` values are not really permitted at the moment, but I think
the InputFormats do not explicitly forbid them. That's why the logic is not
"run until returns null",
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4554
I think what you mentioned is one more reason to not use this in too many
places for now, but only inside the TaskManager / Tasks. Let's introduce that
as a tool that users can use to re
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4525
The logic is currently not correct with the contract of the input formats.
A return value of null is not an "end of split" indicator.
Also, the description mentions that this a
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4554
Ah, I see - that is probably related to reading resources from a jar file
(like the config), methods like `getResourceAsStream()`.
I can see that the "delegate to parent after child&qu
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4554
I took only a very brief look at this, but I am not totally sure whether
the `ChildFirstClassLoader` implementation is actually correct. Even if it is,
it seems to do redundant work, like
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4353#discussion_r133010887
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/TaskStateSnapshot.java
---
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4353#discussion_r133021771
--- Diff:
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinatorTest.java
---
@@ -878,14 +873,17 @@ public void
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4353#discussion_r133013315
--- Diff:
flink-contrib/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBAsyncSnapshotTest.java
---
@@ -164,8
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4353#discussion_r133021720
--- Diff:
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinatorTest.java
---
@@ -850,18 +843,20 @@ public void
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4353#discussion_r133009796
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/TaskStateSnapshot.java
---
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4353#discussion_r133018189
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/StateAssignmentOperation.java
---
@@ -185,44 +184,66 @@ private void
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4353#discussion_r133016663
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/OperatorSubtaskState.java
---
@@ -75,31 +103,79
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4353#discussion_r133022095
--- Diff:
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinatorTest.java
---
@@ -553,31 +551,29 @@ public void
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4353
Concerning the suggestion about the `MultiStreamStateHandle` - I am not
sure that this can always work. Different physical files may have headers, so
it may be important to recognize them as
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4503
Looks pretty good, all in all!
+1 from my side
---
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 StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4353
I had a very rough look at it, and the conceptual rework looks very good.
This would need a detailed pass over the code changes, though, since it
touches very sensitive code...
---
If
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4458
Thanks for picking this up quickly.
I assume there is no way to make this only about `final` modifiers?
We could also approach this by doing selective exclusions of classes (like
the
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4397
@steveloughran Thanks for the comment. We'll take this input into account
for the more elaborate handling in the next version.
---
If your project is set up for it, you can reply to this
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
Final key-words on non-static methods should always be an exception, no
matter whether checkstyle marks them as a violation.
Talking to @NicoK offline, he mentioned that there have been
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
Thinking more about this, I think we should modify the checkstyle to not
force us to remove such `final` keywords. While being redundant in the current
"snapshot" of the code, they
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
If the tooling is hard to be made to cooperate, then I am not religious
about the `final` keyword here.
This is more of a general theme: I am trying to advocate to not change
things
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4445
These changes look good to me!
There is in fact a potential performance impact of this change. It would be
cool to get an understanding of the potential performance impact of only using
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
There is also a complete reorg of a test file, removing try/catch blocks.
This affects to my knowledge only the way where exactly the stack trace is
printed.
I am skeptical about these
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
At the first quick glance: This is removing a lot of `final` keywords from
various methods.
While one could argue that this keyword is not strictly necessary (the
class as a whole is
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4451
Is this really the root of the problem, or is this maybe affected by this:
https://github.com/apache/flink/blob/master/flink-runtime/src/test/java/org/apache/flink/runtime/executiongraph
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4150
@tzulitai Do we shade the `aws-sdk-java` in the Kinesis connector? We
should probably, and shade it in Hadoop as well. If not, this could be a cause
of the conflict...
---
If your project is
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4353#discussion_r130295736
--- Diff:
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/operators/AbstractStreamOperator.java
---
@@ -208,13 +208,13 @@ public
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4397
Want to see how we proceed with master. Same minimal version, or the more
extensive version here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4397
Merged a minimal version of this into `release-1.3` in
854b05376a459a6197e41e141bb28a9befe481ad
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4360
Agree. There are two very non-descriptive commits in the master now.
Let's try to avoid that in the future...
---
If your project is set up for it, you can reply to this email and have
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4397#discussion_r130044644
--- Diff:
flink-core/src/main/java/org/apache/flink/core/fs/ConsistencyLevel.java ---
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4397
How about I just remove the consistency level completely and reduce the
`FileSystemKind` to "file system" and "object store"?
I tried to think through the ecosystem o
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4187#discussion_r129663595
--- Diff:
flink-connectors/flink-connector-kafka-base/src/main/java/org/apache/flink/streaming/connectors/kafka/internals/KafkaCommitCallback.java
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4187#discussion_r129663181
--- Diff:
flink-connectors/flink-connector-kafka-base/src/main/java/org/apache/flink/streaming/connectors/kafka/internals/KafkaCommitCallback.java
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4187#discussion_r129663458
--- Diff:
flink-connectors/flink-connector-kafka-base/src/main/java/org/apache/flink/streaming/connectors/kafka/internals/KafkaCommitCallback.java
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4397
I made the tests stable across Hadoop versions and addressed the
comments/annotations/formatting.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4397#discussion_r129593298
--- Diff:
flink-core/src/main/java/org/apache/flink/core/fs/FileSystemKind.java ---
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4397#discussion_r129592880
--- Diff:
flink-core/src/main/java/org/apache/flink/core/fs/ConsistencyLevel.java ---
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4397#discussion_r129592169
--- Diff:
flink-core/src/main/java/org/apache/flink/core/fs/ConsistencyLevel.java ---
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software
Github user StephanEwen closed the pull request at:
https://github.com/apache/flink/pull/4395
---
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
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4395
Thanks for the review @greghogan , will fix these.
Given the agreement in the mailing list discussion to try it out, I would
like to go ahead and merge this. We should make continuous
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/4397#discussion_r129525270
--- Diff:
flink-core/src/main/java/org/apache/flink/core/fs/ConsistencyLevel.java ---
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software
601 - 700 of 4278 matches
Mail list logo
