New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-5612] Fix GlobPathFilter not-serializable exception #3206

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mushketyk
Contributor

mushketyk commented Jan 25, 2017

Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.

  • General

    • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
    • The pull request addresses only one issue
    • Each commit in the PR has a meaningful commit message (including the JIRA id)
  • Documentation

    • Documentation has been added for new functionality
    • Old documentation affected by the pull request has been updated
    • JavaDoc for public methods has been added
  • Tests & Build

    • Functionality added by the pull request is covered by tests
    • mvn clean verify has been executed successfully locally or a Travis build has passed

Fixed GlobPathFilter serialization exception. As suggested in the JIRA I've made instantiation of PathMatcher's objects lazy to avoid their serialization.

@zentol

zentol approved these changes Jan 25, 2017

Thank you for fixing this so quickly. I've had some small comments, once those are fixed we can merge this.

}
@Test
public void testGlobFilterSerializable() throws IOException {

This comment has been minimized.

@StephanEwen

StephanEwen Jan 25, 2017

Contributor

How about using CommonTestUtils.createCopySerializable() here? That you can validate also that the serialized copy behaves valid.

@StephanEwen

StephanEwen Jan 25, 2017

Contributor

How about using CommonTestUtils.createCopySerializable() here? That you can validate also that the serialized copy behaves valid.

@mushketyk

This comment has been minimized.

Show comment
Hide comment
@mushketyk

mushketyk Jan 25, 2017

Contributor

Hi @StephanEwen, @zentol

Thank you for your comments. I've update the PR accordingly.

Contributor

mushketyk commented Jan 25, 2017

Hi @StephanEwen, @zentol

Thank you for your comments. I've update the PR accordingly.

@zentol

This comment has been minimized.

Show comment
Hide comment
@zentol

zentol Jan 26, 2017

Contributor

+1 to merge.

Contributor

zentol commented Jan 26, 2017

+1 to merge.

@StephanEwen

Two small comments. Would be nice to address them, afterwards this is good to be merged.

@mushketyk

This comment has been minimized.

Show comment
Hide comment
@mushketyk

mushketyk Jan 26, 2017

Contributor

Hi @StephanEwen, @zentol

Thank you for helping with this PR.
I've updated it according to your review.

Contributor

mushketyk commented Jan 26, 2017

Hi @StephanEwen, @zentol

Thank you for helping with this PR.
I've updated it according to your review.

@EronWright

This comment has been minimized.

Show comment
Hide comment
@EronWright

EronWright Jan 26, 2017

Contributor

Just hit this issue, was about to file an bug. Awesome to see this fixed.

It is odd to me that GlobFilePathFilter is marked @Internal.

Contributor

EronWright commented Jan 26, 2017

Just hit this issue, was about to file an bug. Awesome to see this fixed.

It is odd to me that GlobFilePathFilter is marked @Internal.

@StephanEwen

This comment has been minimized.

Show comment
Hide comment
@StephanEwen

StephanEwen Jan 27, 2017

Contributor

@EronWright Agreed, this should be @Public(Evolving).

Contributor

StephanEwen commented Jan 27, 2017

@EronWright Agreed, this should be @Public(Evolving).

@StephanEwen

This comment has been minimized.

Show comment
Hide comment
@StephanEwen

StephanEwen Jan 27, 2017

Contributor

Looks good, will merge this and fix the public annotation on the fly..

Contributor

StephanEwen commented Jan 27, 2017

Looks good, will merge this and fix the public annotation on the fly..

@mushketyk

This comment has been minimized.

Show comment
Hide comment
@mushketyk

mushketyk Jan 27, 2017

Contributor

Thank you @StephanEwen !

Contributor

mushketyk commented Jan 27, 2017

Thank you @StephanEwen !

@zentol

This comment has been minimized.

Show comment
Hide comment
@zentol

zentol Jan 30, 2017

Contributor

@mushketyk The changes have been merged, could you close this PR?

Contributor

zentol commented Jan 30, 2017

@mushketyk The changes have been merged, could you close this PR?

@mushketyk

This comment has been minimized.

Show comment
Hide comment
@mushketyk

mushketyk Jan 30, 2017

Contributor

@zentol Sure. Thank you for merging this!

Contributor

mushketyk commented Jan 30, 2017

@zentol Sure. Thank you for merging this!

@mushketyk mushketyk closed this Jan 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment