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

Allow invalidating SweepStrategyManager cache #4770

Merged
merged 2 commits into from
May 14, 2020

Conversation

mswintermeyer
Copy link
Contributor

Goals (and why):

Useful to change the sweep strategy on an existing table without
bouncing the service.

Implementation Description (bullets):

Just expose a new method that allows cache invalidation.

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

Concerns (what feedback would you like?):

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday):

Useful to change the sweep strategy on an existing table without
bouncing the service.
@mswintermeyer mswintermeyer requested a review from jeremyk-91 May 13, 2020 21:01
@changelog-app
Copy link

changelog-app bot commented May 13, 2020

Generate changelog in changelog/@unreleased

Type

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

Description

Expose a new method on SweepStrategyManager to allow invalidating the table metadata cache on specific tables. Useful for clients that want to change the sweep strategy on an existing table without bouncing the service.

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@gmaretic gmaretic left a comment

Choose a reason for hiding this comment

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

+2
There is technically a race condition when invalidating if we also try to reload at the same time, but I don't think we'd realistically hit it

@Override
public void invalidateCaches(Set<TableReference> tableRefs) {
sweepStrategySupplierLoadingCache.get().invalidateAll(tableRefs);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  /**
   * Discards any cached values for the {@code keys}. The behavior of this operation is undefined
   * for an entry that is being loaded (or reloaded) and is otherwise not present.
   *
   * @param keys the keys whose associated values are to be removed
   * @throws NullPointerException if the specified collection is null or contains a null element
   */
  void invalidateAll(@NonNull Iterable<?> keys);

Invalidating something that's not there looks like it could end up either way. It's a bit nasty, but should we take a read-write lock?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline: The "undefined-ness" here just means that load and invalidate could race and end up either way. This is fine for the internal use case we have planned here.

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!

@jeremyk-91 jeremyk-91 merged commit ffd77e0 into develop May 14, 2020
@delete-merged-branch delete-merged-branch bot deleted the invalidateSweepStrategyManagerCache branch May 14, 2020 17:00
@svc-autorelease
Copy link
Collaborator

Released 0.218.3

@j-baker
Copy link
Contributor

j-baker commented May 19, 2020

This is pretty sketchy because it's not HA safe. Is this a problem?

@mswintermeyer
Copy link
Contributor Author

@j-baker good question. Discussed it with Jeremy, and it's not a problem for our use case at least. The node that calls this is the same node that will delete a cell and thus put it in the thorough sweep queue; the targeted sweeper background thread on a different node might not have its cache invalidated, but it trusts whatever's in the queue and will delete it appropriately. I was imagining that whatever application called this would be responsible for ensuring it gets called on all relevant nodes, but could add docs here to ensure future callers realize the implication. Does that sound reasonable?

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.

5 participants