fix: only use the internal topic id for the shared runtimes without a… #9470
Conversation
…ps://github.com/wcarlson5/ksql into wcarlson5-Do_not_use_the_internal_topics_for_old_runtimes
|
|
| @@ -579,8 +579,7 @@ public static Map<String, Object> buildStreamsProperties( | |||
| ? queryId.get().toString() | |||
| : QueryApplicationId.PERSISTENT_QUERY_INDICATOR; | |||
|
|
|||
| if (config.getBoolean(KsqlConfig.KSQL_SHARED_RUNTIME_ENABLED) | |||
| && type.equals(QueryApplicationId.PERSISTENT_QUERY_INDICATOR)) { | |||
| if (!queryId.isPresent()) { | |||
There was a problem hiding this comment.
This is super confusing, what is the queryId supposed to represent?
IIUC we basically only want to do this if we're building a persistent, and gen 2 query -- it's not at all clear that this is what we're doing here though, I mean why would queryId not be present for only that one specific case?
Also I wonder if we can/should get rid of the type thing that you had introduced in the other PR when we fixed this for transient topics?
There was a problem hiding this comment.
The query Id is to represent the query id. Since this is building a runtime if there is no queryid it is a shared runtime
There was a problem hiding this comment.
Yes we can get rid of the type
| assertThat( | ||
| queryMetadata.getStreamsProperties().get(InternalConfig.TOPIC_PREFIX_ALTERNATIVE), | ||
| is(nullValue()) | ||
| ); |
There was a problem hiding this comment.
I'd suggest that we add a unit test to show precisely what we expect the topic names to be.
Maybe that's a bit prescriptive/specific.
There was a problem hiding this comment.
No we should absolutely do this
also do we have unit tests for all cases/types of queries? Given how many bugs like this have been found so far due to the limited coverage, particularly end-to-end, of ksql unit/integration tests...well, we should aim to fill in all the holes for a given fix/PR
There was a problem hiding this comment.
If we're in a rush to get this hotfix out, which I assume we are, I'm fine with merging this as-is -- but can you follow up with another PR ASAP (so we don't forget) to shore up the test coverage? ie this
|
Merging to pick it for 0.28. Walker can follow up with tests in another PR |
confluentinc#9470) (confluentinc#9475) * fix: only use the internal topic id for the shared runtimes without a query id * add case for prefix alt for old runtime * remove type Co-authored-by: Rohan <[email protected]> Co-authored-by: Walker Carlson <[email protected]> Co-authored-by: Rohan <[email protected]>
only use the internal topic id for the shared runtimes without a query id
Description
What behavior do you want to change, why, how does your patch achieve the changes?
Testing done
Describe the testing strategy. Unit and integration tests are expected for any behavior changes.
Reviewer checklist