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

Atomic support for enhancement #319

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Conversation

metesynnada
Copy link
Contributor

Which issue does this PR close?

Closes 101 and address multiple TODOs inside the codebase.

Rationale for this change

If a large number of states undergo updates, using RwLock for the scheduler's in-memory states at the global level will reduce the overall performance. Additionally, some state updates are not atomic and were listed as TODOs. The modifications in this PR makes concurrency more efficient during heavily loaded in-memory updates and more reliable during state persistence.

What changes are included in this PR?

  • Changed RwLock<HashMap<K, V>> into a DashMap. The dashmap crate provides an implementation of a more sophisticated, high-performance concurrent hash map. For reference, DashMap stats are

    Stars Forks Used By Contributors License
    1.8k stars 104 forks 42.8k 34 MIT license
  • Now performing atomic transactions on TaskManager and ExecuterManager, resolving TODOs.

    • Change in trait StateBackendClient, converted method put_txn to a more general apply_txn.
    • Rollback if the state persisting is not successful.

Are there any user-facing changes?

No

Copy link
Contributor

@thinkharderdev thinkharderdev left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 285 to 291
pub async fn with_locks<Out, F: Future<Output = Out>>(
locks: Vec<Box<dyn Lock>>,
op: F,
) -> Out {
let mut locks = locks;
let result = op.await;
locks.reverse();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
pub async fn with_locks<Out, F: Future<Output = Out>>(
locks: Vec<Box<dyn Lock>>,
op: F,
) -> Out {
let mut locks = locks;
let result = op.await;
locks.reverse();
pub async fn with_locks<Out, F: Future<Output = Out>>(
mut locks: Vec<Box<dyn Lock>>,
op: F,
) -> Out {
let result = op.await;
locks.reverse();

@ozankabak
Copy link
Contributor

Thanks for the review, just sent a commit that incorporates your suggestion.

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.

Leverage Atomic for the in-memory states in Scheduler
4 participants