[controller][compat] Controller part change for supporting separate real-time topic functionality for hybrid stores. #1172
Conversation
haoxu07
force-pushed
the
separateTopicController
branch
from
670f703
to
42639a4
Compare
| @@ -25,7 +25,7 @@ public interface Version extends Comparable<Version>, DataModelBackedStructure<S | |||
| String VERSION_SEPARATOR = "_v"; | |||
| String REAL_TIME_TOPIC_SUFFIX = "_rt"; | |||
| String STREAM_REPROCESSING_TOPIC_SUFFIX = "_sr"; | |||
|
|
|||
| String INCREMENTAL_PUSH_REAL_TIME_TOPIC_SUFFIX = "_rt_sep"; | |||
There was a problem hiding this comment.
Let's follow the existing pattern to separate topic suffix, how about "_inc"?
There was a problem hiding this comment.
Yeah, I set this name due to that is separate real-time topic with lower priority. Incremental push is only one use case at this moment. But just do not want to limit the scope to incremental push only.
| if (setStore.incrementalPushEnabled | ||
| && controllerConfig.enabledSeparateRealTimeTopicForStoreWithIncrementalPush()) { | ||
| setStore.separateRealTimeTopicEnabled = true; | ||
| } |
There was a problem hiding this comment.
Do we need to also add the SEPARATED_TOPIC config update in the updatedConfigsList so that it get populated into child controller?
There was a problem hiding this comment.
Good catch! I will add this.
| partitionCount, | ||
| replicationFactor, | ||
| StoreUtils.getExpectedRetentionTimeInMs(store, hybridStoreConfig.get()), | ||
| false, // Note: do not enable RT compaction! Might make jobs in Online/Offline model stuck |
There was a problem hiding this comment.
nit: looks like the comment is outdated - maybe we can remove it in this PR?
There was a problem hiding this comment.
I am wondering the case if the existing store is upgraded from non-A/A hybrid -> A/A and then enable this config or it is default enabled by cluster level config, but since RT topic has been created before, then this path will be ignored? Do you think it can be move out of this if else block and check separately?
There was a problem hiding this comment.
This is a valid concern. I can add a separate check for this config.
haoxu07
changed the title
haoxu07
force-pushed
the
separateTopicController
branch
from
5604dc7
to
2afaf9e
Compare
haoxu07
force-pushed
the
separateTopicController
branch
from
2afaf9e
to
69426a9
Compare
Summary, imperative, start upper case, don't end with a period
This change add store version level config
separateRealTimeTopicEnabledto allow creating separate real-time topic for isolate bulk real-time traffic in short time range from incremental push. There is cluster level config for enabling this config for all newly converted hybrid store with incremental push enabled.separateRealTimeTopicEnabledis enabled when hybrid enabled store version is added and normal real-time topic is created.Added Integration test to see if incremental push job would send expected traffic to the separate real-time topic after new store is turned this feature.
How was this PR tested?
Does this PR introduce any user-facing changes?