You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "wcarlson5 (via GitHub)" <gi...@apache.org> on 2023/06/08 16:55:08 UTC
[GitHub] [kafka] wcarlson5 opened a new pull request, #13830: KAFKA-14936: Change Time Ordered Buffer to not require Change<> 0/N
wcarlson5 opened a new pull request, #13830:
URL: https://github.com/apache/kafka/pull/13830
Make it so the Time ordered buffer doesn't need a change record. We now have two types for the value. One for the storage type and another for the type. They can be the same if you don't want to use the change value.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] wcarlson5 commented on pull request #13830: KAFKA-14936: Change Time Ordered Buffer to not require Change<> 0/N
Posted by "wcarlson5 (via GitHub)" <gi...@apache.org>.
wcarlson5 commented on PR #13830:
URL: https://github.com/apache/kafka/pull/13830#issuecomment-1585833983
https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-13830/2/tests
Test failures unrelated. As it is a simple type changing PR I'm just going to merge it as is
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] wcarlson5 merged pull request #13830: KAFKA-14936: Change Time Ordered Buffer to not require Change<> 0/N
Posted by "wcarlson5 (via GitHub)" <gi...@apache.org>.
wcarlson5 merged PR #13830:
URL: https://github.com/apache/kafka/pull/13830
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vcrfxia commented on a diff in pull request #13830: KAFKA-14936: Change Time Ordered Buffer to not require Change<> 0/N
Posted by "vcrfxia (via GitHub)" <gi...@apache.org>.
vcrfxia commented on code in PR #13830:
URL: https://github.com/apache/kafka/pull/13830#discussion_r1223656443
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/TimeOrderedKeyValueBuffer.java:
##########
@@ -27,14 +26,14 @@
import java.util.function.Consumer;
import java.util.function.Supplier;
-public interface TimeOrderedKeyValueBuffer<K, V> extends StateStore {
+public interface TimeOrderedKeyValueBuffer<K, V, T> extends StateStore {
Review Comment:
Can we add a quick javadoc to clarify that `T` here is the buffer type? Should already be clear to anyone who reads the code, but a javadoc will help make it so readers won't need to read the code if all they want to know is what the type is.
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryTimeOrderedKeyValueBuffer.java:
##########
@@ -65,7 +65,7 @@
import static org.apache.kafka.streams.state.internals.TimeOrderedKeyValueBufferChangelogDeserializationHelper.deserializeV3;
import static org.apache.kafka.streams.state.internals.TimeOrderedKeyValueBufferChangelogDeserializationHelper.duckTypeV2;
-public final class InMemoryTimeOrderedKeyValueBuffer<K, V> implements TimeOrderedKeyValueBuffer<K, V> {
+public final class InMemoryTimeOrderedKeyValueBuffer<K, V, T> implements TimeOrderedKeyValueBuffer<K, V, Change<V>> {
Review Comment:
Could be good to rename this to `InMemoryTimeOrderedKeyValueChangeBuffer` to contrast with `RocksDBTimeOrderedKeyValueBuffer` (not a change buffer) in your next PR, but I don't feel too strongly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #13830: KAFKA-14936: Change Time Ordered Buffer to not require Change<> 0/N
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #13830:
URL: https://github.com/apache/kafka/pull/13830#discussion_r1226628763
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryTimeOrderedKeyValueChangeBuffer.java:
##########
@@ -418,7 +418,7 @@ public void evictWhile(final Supplier<Boolean> predicate,
changelogTopic,
new Change<>(bufferValue.newValue(), bufferValue.oldValue())
);
- callback.accept(new Eviction<>(key, value, bufferValue.context()));
+ callback.accept(new Eviction<K, Change<V>>(key, value, bufferValue.context()));
Review Comment:
Do you need the type parameters here? My IntelliJ says no, but maybe there is reason that my IntelliJ does not know. Would not be the first time something like this happens.
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryTimeOrderedKeyValueChangeBuffer.java:
##########
@@ -65,7 +65,7 @@
import static org.apache.kafka.streams.state.internals.TimeOrderedKeyValueBufferChangelogDeserializationHelper.deserializeV3;
import static org.apache.kafka.streams.state.internals.TimeOrderedKeyValueBufferChangelogDeserializationHelper.duckTypeV2;
-public final class InMemoryTimeOrderedKeyValueBuffer<K, V> implements TimeOrderedKeyValueBuffer<K, V> {
+public final class InMemoryTimeOrderedKeyValueChangeBuffer<K, V, T> implements TimeOrderedKeyValueBuffer<K, V, Change<V>> {
Review Comment:
Do you need parameter `T` in `InMemoryTimeOrderedKeyValueChangeBuffer<K, V, T>`? It is not used anywhere. I think removing it will reduce the changes in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org