github-actions[bot] commented on code in PR #64133:
URL: https://github.com/apache/doris/pull/64133#discussion_r3569358267
##########
be/src/storage/rowset/group_rowset_writer.cpp:
##########
@@ -68,9 +68,22 @@ Status GroupRowsetWriter::flush_memtable(Block* block,
int32_t segment_id, int64
}
Status GroupRowsetWriter::flush_single_block(const Block* block) {
- RETURN_IF_ERROR(_txn_rowset_writer->flush_single_block(block));
- RETURN_IF_ERROR(_row_binlog_rowset_writer->flush_single_block(block));
+ return flush_single_block(block, allocate_segment_id());
+}
+
+Status GroupRowsetWriter::flush_single_block(const Block* block, int32_t
segment_id) {
+ RETURN_IF_ERROR(_txn_rowset_writer->flush_single_block(block, segment_id));
+ RETURN_IF_ERROR(_row_binlog_rowset_writer->flush_single_block(block,
segment_id));
return Status::OK();
}
+int32_t GroupRowsetWriter::allocate_segment_id() {
Review Comment:
This paired allocation needs to be serialized before the publish path can
use one `GroupRowsetWriter` from multiple calc-delete-bitmap workers. For
row-binlog publish rewrites, `BaseTablet::update_delete_bitmap()` passes the
same group writer into a `CONCURRENT` token when the source rowset has multiple
segments, and each worker calls `allocate_segment_id()`, registers the LSN
vector for that id, then flushes the block with that id. Here the group writer
advances the data writer first and the row-binlog writer second with no shared
lock, so two workers can interleave as data ids 0/1 but row-binlog ids 1/0; the
release build only has a `DCHECK_EQ`. That means the LSN vector can be stored
under a segment id that is consumed by another worker's row-binlog segment, or
missing for the segment being flushed. Please make the paired segment-id
allocation atomic for the two child writers before registering publish-time
row-binlog LSNs.
##########
gensrc/thrift/AgentService.thrift:
##########
@@ -53,6 +53,9 @@ struct TTabletSchema {
24: optional i64 storage_dict_page_size = 262144
25: optional list<Types.TColumnGroup> seq_map
26: optional i32 commit_tso_col_idx = -1
+ 27: optional i32 binlog_tso_idx = -1
Review Comment:
This create-tablet thrift change also needs a rolling-upgrade compatibility
path. A new FE sends the row-binlog indexes only in the new fields 27-29 and
creates the schema as `__DORIS_BINLOG_TSO__`, BIGINT `__DORIS_BINLOG_LSN__`,
`__DORIS_BINLOG_OP__`, while an old BE will ignore those new fields and still
uses the old name/order/type assumptions (`__DORIS_BINLOG_TIMESTAMP__` and
LARGEINT LSN). In the other direction, an old FE sends no fields 27-29, and the
new BE no longer has the old name-based fallback in
`TabletMeta::init_schema_from_thrift()`, so the persisted `TabletSchemaPB`
keeps the row-binlog indexes at `-1`; `TabletSchema::init_from_pb()` copies
those defaults directly and the row-binlog writer later casts/uses invalid
indexes. Please either gate the new schema on BE capability or keep a
create-tablet fallback that recognizes both old and new row-binlog column
layouts and validates the expected types before accepting the tablet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
