Skip to content

[lib] replace config thread-local with global and thread lock#218

Merged
DanSimon merged 1 commit into
developfrom
dsimon/config-thread-lock
Sep 14, 2021
Merged

[lib] replace config thread-local with global and thread lock#218
DanSimon merged 1 commit into
developfrom
dsimon/config-thread-lock

Conversation

@DanSimon

@DanSimon DanSimon commented Sep 7, 2021

Copy link
Copy Markdown
Contributor

When doing some benchmarking on streaming Dataflow, it was observed that if a job was started with large backlog of messages in Pubsub, the job would take a while to start processing messages and seem to have some throughput problems.

It was determined that the underlying cause was twofold.

  1. Dataflow will pull as many messages from pubsub at once, and Beam will attempt to create one thread per message with no upper bound. Thus a large backlog can result in thousands of threads created simultaneously per worker machine.
  2. Klio stores its cached config in a thread-local variable, which means each spawned thread must read the file from disk at least once. Apparently when many threads are started at once, this causes some major I/O contention, which was the source of the lockup and low throughput.

Therefore, this PR replaces the thread-local storage with simply a single global variable guarded by a thread lock. This means that now config is only read from disk once per worker and in a thread-safe manner.

This was tested extensively on Dataflow and with backlogs of over 100k messages, all startup problems were largely eliminated. Beam will still start many threads but they no longer pile up loading config and processing begins shortly after the first thread is spawned.

Only one small issue: I added a simple unit test but due to how we mock out RunConfig in other tests, the test is affected and doesn't run properly. I unsuccessfully made a couple quick attempts to fix, so we'll have to go back and figure out a better way to handle config mocking (or eliminate the need to begin with) if we want to not skip the test.

Checklist for PR author(s)

  • Format the pull request title like [cli] Fixes bugs in 'klio job fake-cmd'.
  • Changes are covered by unit tests (no major decrease in code coverage %) and/or integration tests.
  • Document any relevant additions/changes in the appropriate spot in docs/src.
  • For any change that affects users, update the package's changelog in docs/src/reference/<package>/changelog.rst.

@DanSimon
DanSimon force-pushed the dsimon/config-thread-lock branch from f418831 to 35e7709 Compare September 8, 2021 14:37
Comment thread lib/tests/unit/transforms/test_core.py Outdated
core_transforms.RunConfig._load_config_from_file()


# FIXME: other test files (test_pubsub_message_manager.py and others) patch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just confirming: this is because our tests run in parallel and even resetting the value to None won't guarantee it will be empty when we call .get() if another test loads the config first?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@shireen-bean I suspect (@DanSimon can correct me if I'm wrong) that this is because patches are "leaking" from other tests. FWIW our tests are not run in parallel.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I've gotten around the issue by adding some patcher.stop() calls. I've updated the PR with my changes, tests all pass so it definitely works, though I'm open if there's another approach we should take.

@shireen-bean

Copy link
Copy Markdown
Contributor

The changes and explanations make sense to me! Not going to approve tho because would like multiple sets of eyes before the ✅

@DanSimon
DanSimon force-pushed the dsimon/config-thread-lock branch 4 times, most recently from 7a091f3 to 276581d Compare September 13, 2021 20:22
Comment thread docs/src/reference/lib/changelog.rst Outdated
Changed
*******

* Klio's config file is now read from disk once per worker instead of once per thread (See `PR 218 <https://github.com/spotify/klio/pull/218>` _).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it looks like there's a space between the 2nd back tick and the underscore for the link

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

econchick
econchick previously approved these changes Sep 14, 2021

@econchick econchick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - just need to fix that link in the changelog

@DanSimon
DanSimon force-pushed the dsimon/config-thread-lock branch from 276581d to 6818506 Compare September 14, 2021 15:54
@DanSimon
DanSimon merged commit bd409fc into develop Sep 14, 2021
@DanSimon
DanSimon deleted the dsimon/config-thread-lock branch September 14, 2021 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants