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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
