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-10123] Use ExecutorThreadFactory instead of DefaultThreadFactory in RestServer/Client #6539

Merged
merged 5 commits into from
Aug 15, 2018

Conversation

tillrohrmann
Copy link
Contributor

What is the purpose of the change

Using the ExecutorThreadFactory hardens the system because it uses the FatalExitExceptionHandler
as UncaughtExceptionHandler which terminates the JVM in case of an exception.

Verifying this change

This change is a trivial rework / code cleanup without any 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)
  • The S3 file system connector: (no)

Documentation

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

@yanghua
Copy link
Contributor

yanghua commented Aug 10, 2018

+1

@yanghua
Copy link
Contributor

yanghua commented Aug 11, 2018

Till, scala checkstyle error :

error file=/home/travis/build/apache/flink/flink-runtime/src/main/scala/akka/actor/RobustActorSystem.scala message=File line length exceeds 100 characters line=88

other travis build task also failed, because of some connection timeout.

* limitations under the License.
*/

package akka.actor
Copy link
Contributor

Choose a reason for hiding this comment

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

was this copied from akka?

Copy link
Contributor

Choose a reason for hiding this comment

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

@zentol here we should use this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to use the same package as the Akka classes since ActorSystemImpl is package private private[akka]. It is a bit ugly but it should work.

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 for bafd980

@tillrohrmann
Copy link
Contributor Author

Thanks for the review @zentol and @yanghua. Waiting for Travis to become stable again and after having green light merging this PR.

@yanghua
Copy link
Contributor

yanghua commented Aug 14, 2018

checkstyle error :

08:33:28.933 [ERROR] src/test/java/org/apache/flink/test/recovery/JobManagerHACheckpointRecoveryITCase.java:[22] (imports) ImportOrder: Import org.apache.flink.api.common.functions.RichFlatMapFunction appears after other imports that it should precede

@tillrohrmann
Copy link
Contributor Author

Thanks @yanghua. Fixed the PR. Let's see what Travis says this time :-)

tisonkun and others added 5 commits August 14, 2018 17:10
The RobustActorSystem has a configurable UncaughtExceptionHandler. Per default it is started
with the FatalExitExceptionHandler. This handler terminates the JVM whenever it sees an
uncaught exception.

This closes apache#6334.
…ry in RestServer/Client

Using the ExecutorThreadFactory hardens the system because it uses the FatalExitExceptionHandler
as UncaughtExceptionHandler which terminates the JVM in case of an exception.

This closes apache#6539.
@asfgit asfgit merged commit e307f92 into apache:master Aug 15, 2018
@tillrohrmann tillrohrmann deleted the executorThreadFactory branch June 6, 2019 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants