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

Support for draining workers #12178

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Conversation

kaushik-develop
Copy link
Contributor

@kaushik-develop kaushik-develop commented Sep 24, 2021

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

Support for draining function workers, to scale in a cluster.

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications

Describe the modifications you've done.

  • added new endpoints to drain function assignments off a worker
  • added new endpoints to get the status of a drain operation

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

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

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

  • Added the following to SchedulerManagerTest: testDrain, testGetDrainStatus, testDrainExceptions, testUpdateWorkerDrainMap

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (yes)
    [new endpoints were added under "admin/v2/worker/", related to draining nodes, and getting status of a drain op]
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@kaushik-develop
Copy link
Contributor Author

/pulsarbot run-failure-checks

@kaushik-develop
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Anonymitaet Anonymitaet added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 26, 2021
long startTime = System.nanoTime();
int numRemovedWorkerIds = 0;

if (drainOpStatusMap.size() > 0) {
Copy link
Contributor

@jerrypeng jerrypeng Sep 27, 2021

Choose a reason for hiding this comment

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

I know drainOpStatusMap is a concurrent map but we are reading and writing from different different threads. This makes me nervous. Is there a reason we should synchronize this with drain operation?

Copy link
Contributor Author

@kaushik-develop kaushik-develop Sep 28, 2021

Choose a reason for hiding this comment

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

I didn't quite understand the question: "Is there a reason we should synchronize this with drain operation?"

updateWorkerDrainMap() does a periodic cleanup of stale information about drained workers. We need to do the operation some time after the drain has finished, when the drained worker is removed from the cluster. Since there is currently no hook (that I know of) into the SchedulerManager when a worker is added to, or removed from the cluster, the cleanup is done through a periodic poll (updateWorkerDrainMap).

The drain operation adds a record into the concurrent map [drainOpStatusMap] when a worker is drained. An implicit assumption in the system is that the drained worker will be removed from the system, soon, by an external orchestrator. When the drained worker is seen to be removed from the system, the drainOpStatusMap is cleaned up of stale information in the updateWorkerDrainMap() code.

PLMK if I misunderstood the qs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kaushik-develop there are two independent threads read and updating drainOpStatusMap. One is a schedule periodic task:

https://github.com/apache/pulsar/pull/12178/files#diff-343b3460561c5e794ce7351d663880e37784d37e8c0a877c9e4845b5209a8c84R568

The other is when a drain operation is triggered. Two independent actors can be read and modifying the map concurrently.

Copy link
Contributor Author

@kaushik-develop kaushik-develop Sep 30, 2021

Choose a reason for hiding this comment

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

That is by design. The external orchestrator is expected to trigger the draining of a worker, and check for drain status. It is also expected to remove the worker after a drain operation completes, and not re-add a worker with the same name as the drained/removed worker for one period (configurable, nominally 60 seconds) after the worker-removal. This will ensure that the two actors work on different entries of the concurrent map. But PLMK if you foresee problems in any specific scenario.

@kaushik-develop
Copy link
Contributor Author

/pulsarbot run-failure-checks

@kaushik-develop
Copy link
Contributor Author

/pulsarbot run-failure-checks

@jerrypeng jerrypeng merged commit 7d9e9f3 into apache:master Oct 5, 2021
jerrypeng pushed a commit to jerrypeng/incubator-pulsar that referenced this pull request Nov 4, 2021
Co-authored-by: Kaushik Ghosh <kaushikg@splunk.com>
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
Co-authored-by: Kaushik Ghosh <kaushikg@splunk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/function doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/2.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants