[FLINK-13883] Remove unused AkkaOptions related to Akka's death watch

[tillrohrmann]

What is the purpose of the change

We no longer use Akka's death watch. Hence, this commit removes the death
watch related configuration options AkkaOptions#WATCH_HEARTBEAT_INTERVAL,
AkkaOptions#WATCH_THRESHOLD and AkkaOptions#WATCH_HEARTBEAT_PAUSE.

This PR changes the public evolving API and should, hence, add a release note about the removal of the configuration options.

This PR is based on #9552.

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)

[flinkbot]

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit dac927e (Fri Sep 06 09:08:56 UTC 2019)

✅no warnings

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

• ❓ 1. The [description] looks good.
• ❓ 2. There is [consensus] that the contribution should go into to Flink.
• ❓ 3. Needs [attention] from.
• ❓ 4. The change fits into the overall [architecture].
• ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
• @flinkbot approve all to approve all aspects
• @flinkbot approve-until architecture to approve everything until architecture
• @flinkbot attention @username1 [@username2 ..] to require somebody's attention
• @flinkbot disapprove architecture to remove an approval you gave earlier

[flinkbot]

CI report:

• 9c4b2d8 : CANCELED Build
• dac927e : SUCCESS Build

[zentol]

I'm not quite sold on removing them outright; we have a fair amount of config options that nowadays have no effect, and we opted for deprecation instead of removal.

Unfortunately our handling of deprecated keys isn't very good:
For ConfigOptions(which are read) with an explicit deprecatedKey(s) we already log a warning; however nothing is ever logged when reading/writing options that are just marked as deprecated.
I don't see a good way to handle see, bar defining an array of deprecated keys and checking the config for each of them on cluster startup.

In any case, if the goal is to cause compile errors; well, we could just change the field type to something incompatible. We'd somewhat retain the benefits of deprecation, albeit in a rather odd way.

It is rather unfortunate that we aren't able to add fallback keys for interval and pause to our existing HeartbeatManagerOptions just because the types differ. The proposed FLIP for evolving config options also doesn't seem to address that; although it would be rather trivial to handle if options could have dedicated parsers. @twalthr

[StephanEwen]

+1 to the change

Concerning the point to keep the ConfigOptions for the death watch: I see the point, and it seems to be in line with the handling of the old ConfigConstants class. In this case, I would vote to go for cleanup here, to not accumulate too much dead code.
The keeping of previous options was one of the reasons that got us into the weird state in the first place (referring to unused death watch options for restart strategy defaults).
We still do not break existing config files, we only break programs that refer to the config option fields.

[tillrohrmann]

Thanks for the review @zentol and @StephanEwen.

Thinking about it again, I would slightly lean towards a graceful removal of the configuration options via deprecation. I guess that the main part of the confusion for users comes from this option still being documented. This should no longer be the case once the options are deprecated.

The other problem Stephan raised is that we misused the config option (in the context of the FailureRateRestartStrategy). Deprecating the option won't prevent this in the future to happen again but developers should at least see a deprecation notice telling him not to use it. I hope that this warning would be strong enough in order to prevent the usage of these options until the next release where we could then actually remove the option.

The benefit I see is that we don't break existing programs w/o prior notice (I guess it will be a very small set of affected programs). One could argue that breaking programs would be a kind of notification that one should no longer use this ConfigOption which in itself is a good thing. However, this would not work for custom flink-conf.yamls where this option is specified. Moreover, we will break these programs in the release after the next one.

Can you live with this proposal @zentol and @StephanEwen?

[zentol]

yes.

[tillrohrmann]

Why did you close this PR @zentol?