[server][controller] Add MaterializedViewWriter and support view writers in L/F #1296
Conversation
xunyin8
force-pushed
the
RePartitionViewWriter
branch
2 times, most recently
from
e43850a
to
a711263
Compare
xunyin8
force-pushed
the
RePartitionViewWriter
branch
6 times, most recently
from
791712b
to
bcee9bc
Compare
xunyin8
force-pushed
the
RePartitionViewWriter
branch
from
bcee9bc
to
3c59c8c
Compare
| @@ -329,6 +331,8 @@ public LeaderFollowerStoreIngestionTask( | |||
| for (Map.Entry<String, VeniceViewWriter> viewWriter: viewWriters.entrySet()) { | |||
| if (viewWriter.getValue() instanceof ChangeCaptureViewWriter) { | |||
| tmpValueForHasChangeCaptureViewWriter = true; | |||
| } | |||
There was a problem hiding this comment.
why move the "break" out of the first if statement? The original if statement with break inside seems to neater. Maybe there is something I missed.
| return setProducerOptimizations(internalView.getWriterOptionsBuilder(materializedViewTopicName, version)).build(); | ||
| } | ||
|
|
||
| synchronized private void initializeVeniceWriter() { |
There was a problem hiding this comment.
both ChangeCaptureViewWriter and this writer have the same initWriter method. Can we pull them to their common parent class?
Also, this may be an ask that went too far: instead of using the heavy weight of synchronized, I wonder whether using an AtomicReferenceFieldUpdater with which you don't have to change viewWriter? Or using an atomicReference, with which you will have to change the viewWriter to Reference of ViewWriter?
If the init method is not involved frequently, maybe it is not worthwhile to do such a refactor.
| viewConfigMap = viewConfigMap.entrySet() | ||
| .stream() | ||
| .filter(vc -> Objects.equals(vc.getValue().getViewClassName(), MaterializedView.class.getCanonicalName())) | ||
| .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); | ||
| if (!viewConfigMap.isEmpty()) { | ||
| pushJobSetting.materializedViewConfigFlatMap = ViewUtils.flatViewConfigMapString(viewConfigMap); | ||
| } |
There was a problem hiding this comment.
nice streaming. And maybe you want to combine the !isEmpty to:
pushJobSetting.materializedViewConfigFlatMap = Optional.of (viewConfigMap = viewConfigMap.entrySet()
.stream()
.filter(vc -> Objects.equals(vc.getValue().getViewClassName(), MaterializedView.class.getCanonicalName()))
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)))
.map(ViewUtils::flatViewConfigMapString)
.orElse(Collections::emptyMap);
…ers in L/F
1. View writers will be invoked in L/F SIT too instead of only in A/A SIT. We rely on
view config validation to ensure views that do require A/A are only added to stores
with A/A enabled.
2. This PR only includes creation of materialized view topics, writing of data
records and control messages to the materialized view topics in server and controller.
- Materialized view topics are created during version creation time along with other
view topics.
- SOP is sent during view topic creation time with same chunking and compression
configs as the store version.
- EOP is sent when servers have reported EOP in every partition.
- Incremental push control messages SOIP and EOIP are not propagated to the view topic
for now because the end to end incremental push tracking story for view topics is
not clear yet. Store owners will likely just disable the requirement to wait for
view consumers to fully ingest the incremental push.
- Ingestion heartbeats will be propagated in a broadcast manner. See implementation
for details.
- Version swap for CDC users will be implemented in a separate PR to keep this PR
somewhat short for review.
3. TODO: one pending issue to be resolved is that during processing of batch records
in the native replication source fabric, where we consume local VT, a leader transfer
could result in missing records in the materialized view topic. This is because we
don't do any global checkpointing across leader and followers when consuming local VT.
xunyin8
force-pushed
the
RePartitionViewWriter
branch
from
3c59c8c
to
37c15cc
Compare
| this.viewName = Objects.requireNonNull(viewName, "View name cannot be null for ViewParameters"); | ||
| } | ||
|
|
||
| public Builder(String viewName, Map<String, String> viewParams) { |
There was a problem hiding this comment.
Just a nitpick: An EnumMap can provide a better performance than the Map<String, String> given that all you store is enums.
"A specialized Map implementation for use with enum type keys. All of the keys in an enum map must come from a single enum type that is specified, explicitly or implicitly, when the map is created. Enum maps are represented internally as arrays. This representation is extremely compact and efficient."
| LOGGER.info("Successfully {} for offlinePushStatus: {}", newStatusDetails.toString(), offlinePushStatus); | ||
| } | ||
| } catch (Exception e) { | ||
| String newStatusDetails = "Failed to start EOP procedures"; |
There was a problem hiding this comment.
nit pick: this string is constant. Maybe promote it into a private static variable?
[server][controller] Add MaterializedViewWriter and support view writers in L/F
View writers will be invoked in L/F SIT too instead of only in A/A SIT. We rely on view config validation to ensure views that do require A/A are only added to stores with A/A enabled.
This PR only includes creation of materialized view topics, writing of data records and control messages to the materialized view topics in server and controller.
How was this PR tested?
Unit and integration tests
Does this PR introduce any user-facing changes?