[FLINK-18226][runtime] Fix ActiveResourceManager requesting extra workers on termination of existing workers.

[xintongsong]

What is the purpose of the change

This PR fixes the problem that ActiveResourceManager may request unnecessary extra workers on termination of existing workers.

Brief change log

The equation previously used by ActiveResourceManager for deciding whether and how many new workers to request on termination of existing workers is:
workersToRequest = workersRequiredBySlotManager - workersRequestedByResourceManagerNotAllocated

If there are workers allocated, being started but not yet registered, unnecessary new workers will be requested.

Now the equation is changed to:
workersToRequest = workersRequiredBySlotManager - workersRequestedByResourceManagerNotRegistered

Verifying this change

New test cases added in KubernetesResourceManagerTest and YarnResourceManagerTest.

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, Kubernetes/Yarn/Mesos, ZooKeeper: (yes)
• 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 4123e5f (Fri Jun 12 02:21:00 UTC 2020)

Warnings:

• No documentation files were touched! Remember to keep the Flink docs up to date!

_{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.

Details

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:

• 3da8881 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run travis re-run the last Travis build
• @flinkbot run azure re-run the last Azure build

[tillrohrmann]

Thanks for creating this PR @xintongsong. The changes look good to me. I had some minor comments.

I have to admit that the changes are not super easy to follow with all the different callbacks and the special semantics of allocatedNotRegisteredWorkerResourceSpecs for notifyAllocatedWorkerRegistered and notifyAllocatedWorkerStopped now.

I believe this underlines the need for properly refactoring the ActiveResourceManager. Otherwise I fear that it will become quite unwieldy in the foreseeable future.

[xintongsong]

Thanks for the review, @tillrohrmann.
I've addressed your comments. Please take another look.

BTW. The revert commit is added to clearly show what have been changed since your last review. I'll remove it together with the original renaming commit at merging, if you're good with the changes.

[tillrohrmann]

Thanks for addressing my comments @xintongsong. I had a single comment left which is relevant for calling onTaskManagerRegistration. Please take a look.

[tillrohrmann]

I think we might call onTaskManagerRegistration multiple times if the TM decides to call sendSlotReport repeatedly.

[xintongsong]

Thanks @tillrohrmann, your last comment makes sense to me.
I've updated the PR:

• Rebased onto the latest master branch
• Squashed previous commits
• Addressed your last comments in the new fixup commit.

[tillrohrmann]

Thanks for updating the PR @xintongsong. LGTM. +1 for merging.