You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mjsax (via GitHub)" <gi...@apache.org> on 2026/03/09 03:11:40 UTC
[PR] MINOR: improve generic type safety for IQv2 in metered-store layer [kafka]
mjsax opened a new pull request, #21683:
URL: https://github.com/apache/kafka/pull/21683
(no comment)
--
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
Re: [PR] MINOR: improve generic type safety for IQv2 in metered-store layer [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #21683:
URL: https://github.com/apache/kafka/pull/21683#discussion_r2902979730
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingWindowBytesStoreTest.java:
##########
@@ -64,14 +64,9 @@ public void setUp() {
store.init(context, store);
}
- @AfterEach
- public void tearDown() {
- verify(inner).init(context, store);
Review Comment:
Same
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingTimestampedWindowBytesStoreTest.java:
##########
@@ -65,14 +65,9 @@ public void setUp() {
store.init(context, store);
}
- @AfterEach
- public void tearDown() {
- verify(inner).init(context, store);
Review Comment:
Totally unrelated side cleanup -- but doesn't make any sense to have a `verify` in `@AfterEach` -- this should just be part of the test, which is empty atm and "misuses" the `teatDown()` method to actually test what it should test...
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStore.java:
##########
@@ -106,8 +106,7 @@ public class MeteredKeyValueStore<K, V>
protected NavigableSet<MeteredIterator> openIterators = new ConcurrentSkipListSet<>(Comparator.comparingLong(MeteredIterator::startTimestamp));
- @SuppressWarnings("rawtypes")
- private final Map<Class, QueryHandler> queryHandlers =
+ private final Map<Class<?>, QueryHandler<?>> queryHandlers =
Review Comment:
If we cannot have safe types, make it explicit using `<?>` (similar elsewhere)
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStore.java:
##########
@@ -209,22 +225,23 @@ record -> listener.apply(
@SuppressWarnings("unchecked")
@Override
- public <R> QueryResult<R> query(final Query<R> query,
- final PositionBound positionBound,
- final QueryConfig config) {
-
+ public <R> QueryResult<R> query(
+ final Query<R> query,
+ final PositionBound positionBound,
+ final QueryConfig config
+ ) {
final long start = time.nanoseconds();
final QueryResult<R> result;
- final QueryHandler handler = queryHandlers.get(query.getClass());
+ final QueryHandler<?> handler = queryHandlers.get(query.getClass());
if (handler == null) {
result = wrapped().query(query, positionBound, config);
if (config.isCollectExecutionInfo()) {
result.addExecutionInfo(
"Handled in " + getClass() + " in " + (time.nanoseconds() - start) + "ns");
}
} else {
- result = (QueryResult<R>) handler.apply(
+ result = ((QueryHandler<R>) handler).apply(
Review Comment:
That we have improved type check on the the `QueryHandler`, we need to cast the handler, and get the correct result type automatically.
This is still cleaner. The issue is with the `queryHandlers` Map that introduces the missing types, so the "problem" is not at the right place in the code.
(similar elsewhere in this PR)
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/StoreQueryUtils.java:
##########
@@ -69,17 +69,16 @@ public final class StoreQueryUtils {
* in a map.
*/
@FunctionalInterface
- public interface QueryHandler {
- QueryResult<?> apply(
- final Query<?> query,
+ public interface QueryHandler<R> {
+ QueryResult<R> apply(
+ final Query<R> query,
Review Comment:
This is the key change -- If we get a `Query` with gives us result `R`, and we `apply` it, we should get a `QueryResult<R>` back -- And we thy the result type `R` to the `QueryHandler` interface.
Not using any types here, is unnecessarily loose I believe.
--
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
Re: [PR] MINOR: improve generic type safety for IQv2 in metered-store layer [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax merged PR #21683:
URL: https://github.com/apache/kafka/pull/21683
--
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
Re: [PR] MINOR: improve generic type safety for IQv2 in metered-store layer [kafka]
Posted by "aliehsaeedii (via GitHub)" <gi...@apache.org>.
aliehsaeedii commented on PR #21683:
URL: https://github.com/apache/kafka/pull/21683#issuecomment-4057982134
The branch needs rebasing
--
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