-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-9445: Allow adding changes to allow serving from a specific partition #7984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d1bb6c8
b741148
05d3145
d0a4f53
bd1a270
821e793
1d66675
5c2f99d
42ded2c
e219cbc
87c5b9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -764,7 +764,7 @@ private KafkaStreams(final InternalTopologyBuilder internalTopologyBuilder, | |
| delegatingStateRestoreListener, | ||
| i + 1); | ||
| threadState.put(threads[i].getId(), threads[i].state()); | ||
| storeProviders.add(new StreamThreadStateStoreProvider(threads[i])); | ||
| storeProviders.add(new StreamThreadStateStoreProvider(threads[i], internalTopologyBuilder)); | ||
| } | ||
|
|
||
| final StreamStateListener streamStateListener = new StreamStateListener(threadState, globalThreadState); | ||
|
|
@@ -1160,47 +1160,29 @@ public <K> KeyQueryMetadata queryMetadataForKey(final String storeName, | |
| return streamsMetadataState.getKeyQueryMetadataForKey(storeName, key, partitioner); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Get a facade wrapping the local {@link StateStore} instances with the provided {@code storeName} if the Store's | ||
| * type is accepted by the provided {@link QueryableStoreType#accepts(StateStore) queryableStoreType}. | ||
| * The returned object can be used to query the {@link StateStore} instances. | ||
| * | ||
| * Only permits queries on active replicas of the store (no standbys or restoring replicas). | ||
| * See {@link KafkaStreams#store(java.lang.String, org.apache.kafka.streams.state.QueryableStoreType, boolean)} | ||
| * for the option to set {@code includeStaleStores} to true and trade off consistency in favor of availability. | ||
| * | ||
| * @param storeName name of the store to find | ||
| * @param queryableStoreType accept only stores that are accepted by {@link QueryableStoreType#accepts(StateStore)} | ||
| * @param <T> return type | ||
| * @return A facade wrapping the local {@link StateStore} instances | ||
| * @throws InvalidStateStoreException if Kafka Streams is (re-)initializing or a store with {@code storeName} and | ||
| * {@code queryableStoreType} doesn't exist | ||
| * @deprecated since 2.5 release; use {@link #store(StoreQueryParams)} instead | ||
| */ | ||
| @Deprecated | ||
| public <T> T store(final String storeName, final QueryableStoreType<T> queryableStoreType) { | ||
| return store(storeName, queryableStoreType, false); | ||
| return store(StoreQueryParams.fromNameAndType(storeName, queryableStoreType)); | ||
| } | ||
|
|
||
| /** | ||
| * Get a facade wrapping the local {@link StateStore} instances with the provided {@code storeName} if the Store's | ||
| * Get a facade wrapping the local {@link StateStore} instances with the provided {@link StoreQueryParams}. | ||
| * StoreQueryParams need required parameters to be set, which are {@code storeName} and if | ||
| * type is accepted by the provided {@link QueryableStoreType#accepts(StateStore) queryableStoreType}. | ||
|
brary marked this conversation as resolved.
|
||
| * The returned object can be used to query the {@link StateStore} instances. | ||
|
brary marked this conversation as resolved.
|
||
| * | ||
| * @param storeName name of the store to find | ||
| * @param queryableStoreType accept only stores that are accepted by {@link QueryableStoreType#accepts(StateStore)} | ||
| * @param includeStaleStores If false, only permit queries on the active replica for a partition, and only if the | ||
| * task for that partition is running. I.e., the state store is not a standby replica, | ||
| * and it is not restoring from the changelog. | ||
| * If true, allow queries on standbys and restoring replicas in addition to active ones. | ||
| * @param <T> return type | ||
| * @param storeQueryParams to set the optional parameters to fetch type of stores user wants to fetch when a key is queried | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: remove |
||
| * @return A facade wrapping the local {@link StateStore} instances | ||
| * @throws InvalidStateStoreException if Kafka Streams is (re-)initializing or a store with {@code storeName} and | ||
| * {@code queryableStoreType} doesn't exist | ||
| */ | ||
| public <T> T store(final String storeName, | ||
| final QueryableStoreType<T> queryableStoreType, | ||
| final boolean includeStaleStores) { | ||
| public <T> T store(final StoreQueryParams<T> storeQueryParams) { | ||
| validateIsRunningOrRebalancing(); | ||
| return queryableStoreProvider.getStore(storeName, queryableStoreType, includeStaleStores); | ||
| return queryableStoreProvider.getStore(storeQueryParams); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.kafka.streams; | ||
|
|
||
| import org.apache.kafka.streams.state.QueryableStoreType; | ||
|
|
||
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Represents all the query options that a user can provide to state what kind of stores it is expecting. | ||
|
brary marked this conversation as resolved.
|
||
| * The options would be whether a user would want to enable/disable stale stores | ||
| * or whether it knows the list of partitions that it specifically wants to fetch. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
"list of partitions" -- a user can only specify a single partition (this make we wonder: the optimization we do, only applies to single key lookups? For range queries, a user should never limit the number of partitions? -- if yes, we should explain this detailed in the JavaDocs of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, if a user knows which partition to exactly query for range() or all() or approxnum() apart from get(), withPartition would be helpful for those functions as well. As you said if a data is range partitioned or you specifically want to approx entries in each partition(I do that sometimes to see skew in partitions). I will add this to JavaDocs. |
||
| * If this information is not provided the default behavior is to fetch the stores for all the partitions | ||
|
brary marked this conversation as resolved.
|
||
| * available on that instance for that particular store name. | ||
| * It contains a partition, which for a point queries can be populated from the {@link KeyQueryMetadata}. | ||
|
brary marked this conversation as resolved.
|
||
| */ | ||
| public class StoreQueryParams<T> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As an after though, I am wondering why we not call this class
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I can see that now. @vvcephei wdyt? It being a public class and passed as a part of KIP, can I directly change it in PR? |
||
|
|
||
| private Integer partition; | ||
| private boolean staleStores; | ||
| private final String storeName; | ||
| private final QueryableStoreType<T> queryableStoreType; | ||
|
|
||
| private StoreQueryParams(final String storeName, final QueryableStoreType<T> queryableStoreType) { | ||
| this.storeName = storeName; | ||
| this.queryableStoreType = queryableStoreType; | ||
| } | ||
|
|
||
| public static <T> StoreQueryParams<T> fromNameAndType(final String storeName, | ||
| final QueryableStoreType<T> queryableStoreType) { | ||
| return new StoreQueryParams<T>(storeName, queryableStoreType); | ||
| } | ||
|
|
||
| /** | ||
| * Set a specific partition that should be queried exclusively. | ||
| * | ||
| * @param partition The specific integer partition to be fetched from the stores list by using {@link StoreQueryParams}. | ||
| * | ||
| * @return String storeName | ||
|
brary marked this conversation as resolved.
|
||
| */ | ||
| public StoreQueryParams<T> withPartition(final Integer partition) { | ||
| final StoreQueryParams<T> storeQueryParams = StoreQueryParams.fromNameAndType(this.storeName(), this.queryableStoreType()); | ||
| storeQueryParams.partition = partition; | ||
| storeQueryParams.staleStores = this.staleStores; | ||
| return storeQueryParams; | ||
|
brary marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** | ||
| * Enable querying of stale state stores, i.e., allow to query active tasks during restore as well as standby tasks. | ||
| * | ||
| * @return String storeName | ||
|
brary marked this conversation as resolved.
|
||
| */ | ||
| public StoreQueryParams<T> enableStaleStores() { | ||
| final StoreQueryParams<T> storeQueryParams = StoreQueryParams.fromNameAndType(this.storeName(), this.queryableStoreType()); | ||
| storeQueryParams.partition = this.partition; | ||
| storeQueryParams.staleStores = true; | ||
| return storeQueryParams; | ||
| } | ||
|
|
||
| /** | ||
| * Get the store name for which key is queried by the user. | ||
|
brary marked this conversation as resolved.
|
||
| * | ||
| * @return String storeName | ||
|
brary marked this conversation as resolved.
|
||
| */ | ||
| public String storeName() { | ||
| return storeName; | ||
| } | ||
|
|
||
| /** | ||
| * Get the queryable store type for which key is queried by the user. | ||
| * | ||
| * @return QueryableStoreType queryableStoreType | ||
| */ | ||
|
brary marked this conversation as resolved.
|
||
| public QueryableStoreType<T> queryableStoreType() { | ||
| return queryableStoreType; | ||
| } | ||
|
|
||
| /** | ||
| * Get the partition to be used to fetch list of stores. | ||
|
brary marked this conversation as resolved.
|
||
| * If the method returns {@code null}, it would mean that no specific partition has been requested, | ||
| * so all the local partitions for the store will be returned. | ||
|
brary marked this conversation as resolved.
|
||
| * | ||
| * @return Integer partition | ||
| */ | ||
| public Integer partition() { | ||
| return partition; | ||
| } | ||
|
|
||
| /** | ||
| * Get the flag staleStores. If {@code true}, include standbys and recovering stores along with running stores. | ||
| * | ||
| * @return boolean staleStores | ||
| */ | ||
| public boolean staleStoresEnabled() { | ||
| return staleStores; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object obj) { | ||
| if (!(obj instanceof StoreQueryParams)) { | ||
| return false; | ||
| } | ||
| final StoreQueryParams storeQueryParams = (StoreQueryParams) obj; | ||
| return Objects.equals(storeQueryParams.partition, partition) | ||
| && Objects.equals(storeQueryParams.staleStores, staleStores) | ||
| && Objects.equals(storeQueryParams.storeName, storeName) | ||
| && Objects.equals(storeQueryParams.queryableStoreType, queryableStoreType); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "StoreQueryParams {" + | ||
| "partition=" + partition + | ||
| ", staleStores=" + staleStores + | ||
| ", storeName=" + storeName + | ||
| ", queryableStoreType=" + queryableStoreType + | ||
| '}'; | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(partition, staleStores, storeName, queryableStoreType); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,9 +16,11 @@ | |
| */ | ||
| package org.apache.kafka.streams.state.internals; | ||
|
|
||
| import org.apache.kafka.streams.StoreQueryParams; | ||
| import org.apache.kafka.streams.errors.InvalidStateStoreException; | ||
| import org.apache.kafka.streams.processor.StateStore; | ||
| import org.apache.kafka.streams.processor.TaskId; | ||
| import org.apache.kafka.streams.processor.internals.InternalTopologyBuilder; | ||
| import org.apache.kafka.streams.processor.internals.StreamThread; | ||
| import org.apache.kafka.streams.processor.internals.Task; | ||
| import org.apache.kafka.streams.state.QueryableStoreType; | ||
|
|
@@ -30,27 +32,36 @@ | |
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class StreamThreadStateStoreProvider { | ||
|
|
||
| private final StreamThread streamThread; | ||
| private final InternalTopologyBuilder internalTopologyBuilder; | ||
|
|
||
| public StreamThreadStateStoreProvider(final StreamThread streamThread) { | ||
| public StreamThreadStateStoreProvider(final StreamThread streamThread, | ||
| final InternalTopologyBuilder internalTopologyBuilder) { | ||
| this.streamThread = streamThread; | ||
| this.internalTopologyBuilder = internalTopologyBuilder; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public <T> List<T> stores(final String storeName, | ||
| final QueryableStoreType<T> queryableStoreType, | ||
| final boolean includeStaleStores) { | ||
| public <T> List<T> stores(final StoreQueryParams storeQueryParams) { | ||
| final String storeName = storeQueryParams.storeName(); | ||
| final QueryableStoreType<T> queryableStoreType = storeQueryParams.queryableStoreType(); | ||
| final TaskId keyTaskId = createKeyTaskId(storeName, storeQueryParams.partition()); | ||
| if (streamThread.state() == StreamThread.State.DEAD) { | ||
| return Collections.emptyList(); | ||
| } | ||
| final StreamThread.State state = streamThread.state(); | ||
| if (includeStaleStores ? state.isAlive() : state == StreamThread.State.RUNNING) { | ||
| final Map<TaskId, ? extends Task> tasks = includeStaleStores ? streamThread.allTasks() : streamThread.activeTasks(); | ||
| if (storeQueryParams.staleStoresEnabled() ? state.isAlive() : state == StreamThread.State.RUNNING) { | ||
| final Map<TaskId, ? extends Task> tasks = storeQueryParams.staleStoresEnabled() ? streamThread.allTasks() : streamThread.activeTasks(); | ||
| final List<T> stores = new ArrayList<>(); | ||
| for (final Task streamTask : tasks.values()) { | ||
| if (keyTaskId != null && !keyTaskId.equals(streamTask.id())) { | ||
| continue; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have this check inside the loop? Should we not check this condition once, and only execute the loop if the condition is Or maybe pass a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems you missed this commnet? If you disagree with my comment, can you elaborate why (maybe I am missing something)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If keyTaskId is null(user wants all servable stores) -> loop over all tasks and return the list of stores. So, probably I can write it as: But I feel the current code is more readable than this one.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do loop over all tasks? If a user want to query a single partition, we can just lookup the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| final StateStore store = streamTask.getStore(storeName); | ||
| if (store != null && queryableStoreType.accepts(store)) { | ||
| if (!store.isOpen()) { | ||
|
|
@@ -72,8 +83,23 @@ public <T> List<T> stores(final String storeName, | |
| } else { | ||
| throw new InvalidStateStoreException("Cannot get state store " + storeName + " because the stream thread is " + | ||
| state + ", not RUNNING" + | ||
| (includeStaleStores ? " or REBALANCING" : "")); | ||
| (storeQueryParams.staleStoresEnabled() ? " or REBALANCING" : "")); | ||
| } | ||
| } | ||
|
|
||
| private TaskId createKeyTaskId(final String storeName, final Integer partition) { | ||
| if (partition == null) { | ||
|
brary marked this conversation as resolved.
|
||
| return null; | ||
| } | ||
| final List<String> sourceTopics = internalTopologyBuilder.stateStoreNameToSourceTopics().get(storeName); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we only need the source topics names and topic groups, we should not pass in the full
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However the below that both here and This is just a code style suggestion.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @guozhangwang -- as this is internal, we can still improve on it in a follow up.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @guozhangwang and @mjsax on suggestions here. Now, since these can change during rebalancing, I am assuming just passing source topics names and topic groups will not suffice and we will have to pass complete internalTopologyBuilder. Is that right? I have added |
||
| final Set<String> sourceTopicsSet = sourceTopics.stream().collect(Collectors.toSet()); | ||
| final Map<Integer, InternalTopologyBuilder.TopicsInfo> topicGroups = internalTopologyBuilder.topicGroups(); | ||
| for (final Map.Entry<Integer, InternalTopologyBuilder.TopicsInfo> topicGroup : topicGroups.entrySet()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks right to me.. (I can't be sure unless I step through the code though :))
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added an integration test covering all 4 cases.
|
||
| if (topicGroup.getValue().sourceTopics.containsAll(sourceTopicsSet)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we are only interested in the source topic ber topicGroup, we should just pass a |
||
| return new TaskId(topicGroup.getKey(), partition.intValue()); | ||
| } | ||
| } | ||
| throw new InvalidStateStoreException("Cannot get state store " + storeName + " because the requested partition " + partition + "is" + | ||
| "not available on this instance"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: missing whitespace ->
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. \cc @vitojeng -- seems this is a new case we should cover in KIP-216?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should not need update KIP-216. Does this make sense?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For line 66-71 above, we will throw Discussing this question, I am wondering if we should still cover the case when the user passes in a partition number that is not available? This would be a user error similar to passing in a wrong store name?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh...I just realized what you meant. Yes, we should cover this case in KIP-216, will update KIP. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| */ | ||
| package org.apache.kafka.streams.state.internals; | ||
|
|
||
| import org.apache.kafka.streams.StoreQueryParams; | ||
| import org.apache.kafka.streams.errors.InvalidStateStoreException; | ||
| import org.apache.kafka.streams.state.QueryableStoreType; | ||
|
|
||
|
|
@@ -28,20 +29,25 @@ | |
| public class WrappingStoreProvider implements StateStoreProvider { | ||
|
|
||
| private final List<StreamThreadStateStoreProvider> storeProviders; | ||
| private final boolean includeStaleStores; | ||
| private StoreQueryParams storeQueryParams; | ||
|
|
||
| WrappingStoreProvider(final List<StreamThreadStateStoreProvider> storeProviders, | ||
| final boolean includeStaleStores) { | ||
| final StoreQueryParams storeQueryParams) { | ||
| this.storeProviders = storeProviders; | ||
| this.includeStaleStores = includeStaleStores; | ||
| this.storeQueryParams = storeQueryParams; | ||
| } | ||
|
|
||
| //visible for testing | ||
| public void setStoreQueryParams(final StoreQueryParams storeQueryParams) { | ||
| this.storeQueryParams = storeQueryParams; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this setter? Why can't we pass in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There were four different tests in this class, with different storeName or queryable store type for each test. So, I would have to create 4 different instances of wrappingStoreProvider in the #before (one for each test) if I don't add a setter. I thought this to easier. I can surely do that as well. LMK.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it would be cleaner to create a new instance for each test -- if you test |
||
|
|
||
| @Override | ||
| public <T> List<T> stores(final String storeName, | ||
| final QueryableStoreType<T> queryableStoreType) { | ||
| final List<T> allStores = new ArrayList<>(); | ||
| for (final StreamThreadStateStoreProvider provider : storeProviders) { | ||
| final List<T> stores = provider.stores(storeName, queryableStoreType, includeStaleStores); | ||
| final List<T> stores = provider.stores(storeQueryParams); | ||
| allStores.addAll(stores); | ||
| } | ||
| if (allStores.isEmpty()) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.