MINOR: Include all hosts in metadata for topology#12594
Conversation
…st is empty as it is a common way to find the other host addresses
vvcephei
left a comment
There was a problem hiding this comment.
Thanks, @wcarlson5 !
This fix looks good to me. I'm +1 when the tests pass.
I did have a few suggestions for improving the test coverage.
| // then verify that only this topology's one store appears if the host has partitions assigned | ||
| if (!metadata.topicPartitions().isEmpty()) { | ||
| assertThat(metadata.stateStoreNames(), equalTo(singleton("store-" + topologyName))); | ||
| } |
There was a problem hiding this comment.
It seems like this test is generally missing the converse:
In addition to checking if all the assigned topics are in the named topology sources, should we also check that all named topology sources are assigned?
Likewise, in this new block, should we assert that the store is not present if the partition is not assigned?
There was a problem hiding this comment.
Well, not all named topology topics are going to be in all the source metadata, we can check the stores are limited to the relevant topics. This might just be useful for is the topic partitions are empty.
| assertThat(streams.allStreamsClientsMetadataForTopology(TOPOLOGY_2).size(), equalTo(2)); | ||
| assertThat(streams2.allStreamsClientsMetadataForTopology(TOPOLOGY_2).size(), equalTo(2)); |
There was a problem hiding this comment.
Should we also verify that topology1 metadata includes the full set of hosts?
|
|
@vvcephei Wow every single test passed. Don't see that very often on a pr |
ableegoldman
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix
| standbyStoresOnHost.addAll(getStoresOnHost(storeToSourceTopics, standbyPartitionsOnHost)); | ||
| } | ||
|
|
||
| if (!(activeStoresOnHost.isEmpty() && activePartitionsOnHost.isEmpty() && standbyStoresOnHost.isEmpty() && standbyPartitionsOnHost.isEmpty())) { |
There was a problem hiding this comment.
I'm trying to remember what the original intention behind this check was, I imagine it was to avoid rebuilding the metadata unnecessarily and that we did not have this check before named topologies because this only becomes nontrivial as the metadata rebuild cost scales with the number of topologies...?
That said, wouldn't we still want/need to update the metadata if it's become empty? It seems like we should actually only ever skip it when the delta is zero/empty, not when the current set is empty...but I guess that's the root of this fix to begin with. Anyways we should err on the side of correctness rather than performance issues, especially perf issues we haven't seen/confirmed to exist
Yeah that is legitimately amazing. Love to see it! Maybe things are turning around...heh... 😭 |
When building streams metadata we want to build even if the host is empty as it is a common way to find the other host addresses Reviewers: John Roesler <vvcephei@apache.org>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>
When building streams metadata we want to build even if the host is empty as it is a common way to find the other host addresses Reviewers: John Roesler <vvcephei@apache.org>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>
When building streams metadata we want to build even if the host is empty as it is a common way to find the other host addresses Reviewers: John Roesler <vvcephei@apache.org>, Anna Sophie Blee-Goldman <ableegoldman@apache.org> Co-authored-by: Walker Carlson <18128741+wcarlson5@users.noreply.github.com>
When building streams metadata we want to build even if the host is empty as it is a common way to find the other host addresses
update existing tests to find all hosts
Committer Checklist (excluded from commit message)