Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

[CCB] | MultiClientCommitTimestampGetter refactor - part 2 #5294

Merged
merged 11 commits into from
Mar 3, 2021

Conversation

sudiksha27
Copy link
Contributor

@sudiksha27 sudiksha27 commented Mar 2, 2021

Goals (and why):
Atlas should be able to accept a common batcher for getCommitTimestamps requests for multiple clients.

Implementation Description (bullets):

  • Implements AutoBatcher capable of handling getCommitTimestamps requests for multiple clients
  • The model is very close to the origin model - for each namespace, while all requests cannot be served, we batch all the requests together, try to do getCommitTimestamps in one call to the server & update the cache after receiving the response.
  • The LockWatchEventCache#lastKnownVersion is fetched before making request to TimeLock and LockWatchEventCache#processGetCommitTimestampsUpdate is invoked as response is received.
  • We try to serve requests as responses become available instead of waiting for all responses to serve all requests in one go.
  • In case of exception, all pending requests are cancelled.
  • This PR actually does not change the current flow
  • I haven't done the wiring yet as the PR is pretty big already

Testing (What was existing testing like? What have you done to improve it?):
Added tests

Concerns (what feedback would you like?):

  • There is code duplication b/w NamespacedBatchStateManager#process and BatchingCommitTimestampGetter#process but I could not think of a way of refactoring without 1. adding another iteration over request list 2. creating new instances of a suitable DS
  • Did I miss/ break something horribly?

Where should we start reviewing?:
MultiClientCommitTimestampGetter

Priority (whenever / two weeks / yesterday):
Before EOD tomorrow is the dream

@changelog-app
Copy link

changelog-app bot commented Mar 2, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

AtlasDb client can now batch getCommitTimestamps requests across namespaces.

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

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

🕐 0:45 🏁 0:58

The core logic does look equivalent to a valid parallel execution of the current BatchingCommitTimetsampGetters. My comments here are largely around factoring.

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

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

👍 looks good, thanks for fixing all the comments!

this.pendingFutures = new LinkedList<>();
this.transientResponseList = new LinkedList<>();
this.pendingFutures = new ArrayDeque<>();
this.transientResponseList = new ArrayDeque<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@bulldozer-bot bulldozer-bot bot merged commit 7f17418 into develop Mar 3, 2021
@bulldozer-bot bulldozer-bot bot deleted the ccb-commitTs-2 branch March 3, 2021 13:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants