Skip to content

[WIP] demonstrating my thoughts on #6192#6200

Closed
vvcephei wants to merge 2 commits intoapache:trunkfrom
vvcephei:pr-6192-john
Closed

[WIP] demonstrating my thoughts on #6192#6200
vvcephei wants to merge 2 commits intoapache:trunkfrom
vvcephei:pr-6192-john

Conversation

@vvcephei
Copy link
Copy Markdown
Contributor

demonstrating my thoughts on #6192

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@vvcephei
Copy link
Copy Markdown
Contributor Author

I've only really sketched out the key/value implementation.

At a high level:

  • no more record converter. It was only used by state restore, and now it's purely handled by the base-level store. The actual conversion still takes place, and the logic is still spread out in too many places, but I think it can be consolidated into StoreProxyUtils.
  • as an invariant, all stores that the DSL registers appear to be plain KeyValueStore<K,V>.
  • as an invariant, all stores that the DSL registers are actually KeyValueStoreFacades<K,V>, which get unpacked in by the processors that care.
  • even if you call put(key, value) on an apparent KeyValueStore, the base-level RocksKeyValueTimestamp store looks up the current timestamp in the context
  • the changelog wrapper and caching wrapper are simplified, by intercepting the byte arrays only at the point where they actually need to be translated (right before forwarding or logging) instead of duplicating any processing logic

WDYT, @mjsax ?

@vvcephei
Copy link
Copy Markdown
Contributor Author

Oh, and there are some more general improvements that I made to help myself have some confidence that we're not breaking everything with this change...

  • the WrappingStore definition now has a generic parameter, so we can be sure that the wrapped store has the same basic type as the wrapping store. For example, we don't want to wrap a window store inside a key/value store, but the pre-existing code permits this.
  • the AbstractStore abstract implementation in WrappingStore also makes use of this generic information to return the proper type of the inner store, which means that it can be used for all stores that wrap other stores, which means less finicky logic around what you're getting when you unwrap a wrapped store.
  • I renamed some of the new classes to clarify their role
  • I renamed some stuff that was apparently completely misnamed and confused me (like in DirtyCacheEntry, which stored an LRUCacheEntry, but called it a "record context" :/ )

@vvcephei
Copy link
Copy Markdown
Contributor Author

Just in case anyone else stumbles across this PR, it's a vehicle to express some thoughts to Matthias. After a discussion, he'll take whatever ideas he likes and incorporate them into his PRs. Anything more general that's still of value, I'll send in independent follow-up PRs.

Once both of these functions are completed, we'll close this PR and forget all about it ;)

@github-actions
Copy link
Copy Markdown

This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

@github-actions github-actions Bot added the stale Stale PRs label Nov 21, 2024
@github-actions
Copy link
Copy Markdown

This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.

@github-actions github-actions Bot added the closed-stale PRs that were closed due to inactivity label Dec 22, 2024
@github-actions github-actions Bot closed this Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

closed-stale PRs that were closed due to inactivity stale Stale PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants