Skip to content
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-7311][tests] refrain from using fail(Exception#getMessage()) #4446

Closed
wants to merge 1 commit into from

Conversation

NicoK
Copy link
Contributor

@NicoK NicoK commented Aug 1, 2017

What is the purpose of the change

Refrain from using fail(Exception#getMessage()) because it hides the exception's stack trace and is bad style.

This PR is based on #4445.

Brief change log

  • remove code pattern fail(Exception#getMessage()) and let the tests throw exceptions instead

Verifying this change

This change is a trivial rework / code cleanup without any new test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@NicoK NicoK changed the title [FLINK-7311][tests] refrain from using fail(Exception#getMessage()) [FLINK-7311][tests] refrain from using fail(Exception#getMessage()) Aug 1, 2017
@zentol
Copy link
Contributor

zentol commented Aug 1, 2017

I will review this in the evening.

Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1.

5 minutes well spent.

@NicoK
Copy link
Contributor Author

NicoK commented Aug 2, 2017

actually, the FLINK-7311 commit also cleanly applies to master (without FLINK-7310 that this PR is based on). Should I rebase?

@zentol
Copy link
Contributor

zentol commented Aug 2, 2017

If FLINK-7310 also cleanly applies to FLINK-7311 as well, i would say yes. If not it's up to you whether you want to invest the time for a rebase.

@zentol
Copy link
Contributor

zentol commented Aug 2, 2017

merging.

zentol pushed a commit to zentol/flink that referenced this pull request Aug 2, 2017
@asfgit asfgit closed this in e83217b Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants