Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@mxm @aljoscha @StephanEwen Thank you for your comments. Seems like this is
sorted out :)
I'll remove Guava calls today at the evening.
---
If your project is set up for it, you can reply to
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2109
Ah, got it too now. The diff only makes it appear like the deprecated
method is new. Then I take that back :-)
A followup on removing Guava would be nice, through...
---
If your project is
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2109
@mxm @mushketyk Technically this change does add a new method that is the
now deprecated method minus the `FilePathFilter`. That's a good change, though,
since it simplifies the API in the long
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2109
@aljoscha As already pointed out in the JIRA issue, the method is not new.
A now obsolete method has been deprecated (the diff is hard to read).
---
If your project is set up for it, you can reply to
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2109
Hi,
I just saw that this was merged but I still have some comments. What is the
reason for the added Guava (test)-dependency? (Probably a leftover but should
nevertheless not be in this) And
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@mxm Thank you for your reviews and for merging 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
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2109
Merging this and addressing the last comment.
---
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 mxm commented on the issue:
https://github.com/apache/flink/pull/2109
Thanks @mushketyk. Good to merge after the final fix.
---
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
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@mxm Thank you for the review. I've updated PR accordingly.
---
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 mxm commented on the issue:
https://github.com/apache/flink/pull/2109
Left some more comments but I think this is pretty much ready to be merged.
---
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
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@mxm Sorry for reminding you. I just thought that everybody forgot about
this PR at some point :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2109
@mushketyk Sorry that we make you wait. I already had a look. I'll continue
now.
---
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
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
This PR was not reviewed in the last 21 days. Could someone please review
it? It should be in a good shape at this stage.
---
If your project is set up for it, you can reply to this email and
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@mxm @zentol @kl0u Could you please give it another review?
---
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 mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
The build was failing because Jenkins couldn't merge my changes. I rebased
them to current master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@mxm I've updated the PR according to your comments.
---
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 mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@mxm Thank you for your detailed review! I'll update it according to your
feedback and revert the breaking changes.
---
If your project is set up for it, you can reply to this email and have your
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2109
Just had a brief look. Looks nice. Actually, I like how you moved the file
filter to the `FileInputFormat` instead of having it as a special case of the
file monitoring source.
I think you
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2109
I'll try to review this as soon as possible. Maybe @kl0u or @zentol also
want to have another look.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
Could someone please review this again?
---
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 mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
Fixed the bug, now build is passing.
---
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 mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
Apparently my change brakes one of the integration tests. I'll fix it
promptly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
Rebased on top of master branch.
---
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
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
Hi @kl0u . Thank you for your code review.
I've updated my code according to the 2nd and 3rd suggestions that you
provided, but I am not sure how I can replace DefaultFilter implementation with
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/2109
1) If it is to include it, then I would suggest to just replace the
DefaultFilter with that, and probably give some initial patterns for e.g.
hidden files (start with "."). Having two implementations,
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2109
1. Technically the GlobFilePathFilter is not redundant, since it fulfills
exactly the requirements of the issue; be able to filter files by specifying a
regular expression. It also offers
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/2109
Hi @mushketyk ,
Sorry for the late reply.
I have some general comments on the PR.
1) Given the FilePathFilter, I think that the GlobFilePathFilter is
redundant, right? It is a
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/2109
@mushketyk Hello! Sorry for the later reply.
I can review this on Monday. This week I am on holidays.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@zentol @kl0u Is there anything else I should change in this PR?
---
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
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@kl0u
>> and pass the FilePathFilter as an argument to the constructor of the
FileInputFormat and then do the filtering the same way you do it
I removed configuration override, but added a
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/2109
Hello @mushketyk and sorry for the late response.
Great that you are working on that also for the Batch API!
Recently we introduced in the Streaming API (not batch) the notion of
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@kl0u I've updated my PR to use FilePathFilter abstract class. I had to
move it to flink-core to avoid circular dependency between packages.
---
If your project is set up for it, you can reply to
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
@kl0u Thank you for your comment. I'll update my PR.
Do you have any thoughts regarding questions raised by @zentol ?
Specifically how to provide include/exclude patterns?
---
If your project
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/2109
Recently we added the FilePathFilter abstract class that is now used by
some user-facing functions for exactly the same reason. I would suggest that
this abstraction is also used by this PR, so that we
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2109
There are a number of unstable tests that we are aware of. Rebasing to the
latest master can reduce this issues, but they aren't completely resolved yet.
---
If your project is set up for it, you
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
By the way, build in CI seems to fail pretty much randomly. Do I need to
rebase my changes to a later revision to solve this?
---
If your project is set up for it, you can reply to this email
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
Ok, I'll update my PR when others will comment on it.
---
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 zentol commented on the issue:
https://github.com/apache/flink/pull/2109
yes i think so.
I have 2 more issues with this PR.
For one the documentation isn't in the correct place i believe. The
config.md file specifies configuration parameters for Flink as
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
I've added a test for an HDFS file. Does this address your comment?
---
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
Github user mushketyk commented on the issue:
https://github.com/apache/flink/pull/2109
Fixed my PR according to the review.
---
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 zentol commented on the issue:
https://github.com/apache/flink/pull/2109
You marked this PR as containing documentation for new functionality, but i
can't find any changes to the docs regarding the newly added configuration
keys. Did you forget to include them in this PR?
41 matches
Mail list logo
