This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 1c69ec608 [tablet] fix race in RowSetMetadata::id()
1c69ec608 is described below
commit 1c69ec6081a88dc531ebaa60b19abd4c0e54ae47
Author: Alexey Serbin <[email protected]>
AuthorDate: Tue Sep 10 09:42:55 2024 -0700
[tablet] fix race in RowSetMetadata::id()
This patch fixes a race in access to the RowSetMetadata::id_ field
in the rollback scenario in the MajorCompactDeltaStoresWithColumnIds()
method of the DiskRowSet class.
Before this patch, TSAN would report warnings like below when running
the MultiThreadedHybridClockTabletTest.UpdateNoMergeCompaction scenario:
of the mt-tablet-test:
Read of size 8 at 0x7b3400014780 by thread T30 (mutexes: write
M76293278759445
9152, write M7098002):
#0 kudu::tablet::RowSetMetadata::id() const
src/kudu/tablet/rowset_metadata.h:100:31 (libtablet.so+0x346faa)
#1 kudu::tablet::RowSetTree::Reset(...)
src/kudu/tablet/rowset_tree.cc:190:48 (libtablet.so+0x4bf666)
#2 kudu::tablet::Tablet::ModifyRowSetTree(...)
src/kudu/tablet/tablet.cc:1490:3 (libtablet.so+0x323755)
#3 kudu::tablet::Tablet::AtomicSwapRowSetsUnlocked(...)
src/kudu/tablet/tablet.cc:1504:3 (libtablet.so+0x3239bc)
#4 kudu::tablet::Tablet::AtomicSwapRowSets(...)
src/kudu/tablet/tablet.cc:1496:3 (libtablet.so+0x3238f9)
...
Previous write of size 8 at 0x7b3400014780 by thread T12 (mutexes:
write M625572878699880144, write M530715863088620288, write
M525367769810683784):
#0 kudu::tablet::RowSetMetadata::LoadFromPB(...)
src/kudu/tablet/rowset_metadata.cc:77:7 (libtablet.so+0x4f9f03)
#1
kudu::tablet::DiskRowSet::MajorCompactDeltaStoresWithColumnIds(...)::$_0::operator()()
const src/kudu/tablet/diskrowset.cc:603:23 (libtablet.so+0x46eddf)
#2
kudu::ScopedCleanup<kudu::tablet::DiskRowSet::MajorCompactDeltaStoresWithColumnIds(...)::$_0>::~ScopedCleanup()
src/kudu/util/scoped_cleanup.h:51:7 (libtablet.so+0x46cc5a)
#3
kudu::tablet::DiskRowSet::MajorCompactDeltaStoresWithColumnIds(...)
src/kudu/tablet/diskrowset.cc:636:1 (libtablet.so+0x46c5c9)
#4 kudu::tablet::DiskRowSet::MajorCompactDeltaStores(...)
src/kudu/tablet/diskrowset.cc:570:10 (libtablet.so+0x46c013)
...
SUMMARY: ThreadSanitizer: data race
src/kudu/tablet/rowset_metadata.h:100:31 in kudu::tablet::RowSetMetadata::id()
const
Change-Id: I4b09575616e754b7dbb24586293f128e361b9360
Reviewed-on: http://gerrit.cloudera.org:8080/21779
Reviewed-by: Mahesh Reddy <[email protected]>
Tested-by: Alexey Serbin <[email protected]>
Reviewed-by: Yingchun Lai <[email protected]>
---
src/kudu/tablet/cfile_set.cc | 10 ++++------
src/kudu/tablet/rowset_metadata.cc | 12 ++++++++----
src/kudu/tablet/rowset_metadata.h | 10 ++++++++--
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/kudu/tablet/cfile_set.cc b/src/kudu/tablet/cfile_set.cc
index 48ed9e26f..1bcdfaaea 100644
--- a/src/kudu/tablet/cfile_set.cc
+++ b/src/kudu/tablet/cfile_set.cc
@@ -137,18 +137,16 @@ Status CFileSet::DoOpen(const IOContext* io_context) {
// Lazily open the column data cfiles. Each one will be fully opened
// later, when the first iterator seeks for the first time.
- RowSetMetadata::ColumnIdToBlockIdMap block_map =
rowset_metadata_->GetColumnBlocksById();
- for (const RowSetMetadata::ColumnIdToBlockIdMap::value_type& e : block_map) {
- ColumnId col_id = e.first;
- DCHECK(!ContainsKey(readers_by_col_id_, col_id)) << "already open";
-
+ const RowSetMetadata::ColumnIdToBlockIdMap block_map =
rowset_metadata_->GetColumnBlocksById();
+ for (const auto& [col_id, _] : block_map) {
unique_ptr<CFileReader> reader;
RETURN_NOT_OK(OpenReader(rowset_metadata_->fs_manager(),
cfile_reader_tracker_,
rowset_metadata_->column_data_block_for_col_id(col_id),
io_context,
&reader));
- readers_by_col_id_[col_id] = std::move(reader);
+ const auto& ins_info = readers_by_col_id_.emplace(col_id,
std::move(reader));
+ DCHECK(ins_info.second) << "already open";
VLOG(1) << "Successfully opened cfile for column id " << col_id
<< " in " << rowset_metadata_->ToString();
}
diff --git a/src/kudu/tablet/rowset_metadata.cc
b/src/kudu/tablet/rowset_metadata.cc
index c14d2ca97..463c9489f 100644
--- a/src/kudu/tablet/rowset_metadata.cc
+++ b/src/kudu/tablet/rowset_metadata.cc
@@ -64,7 +64,7 @@ Status RowSetMetadata::Flush() {
}
Status RowSetMetadata::InitFromPB(const RowSetDataPB& pb) {
- CHECK(!initted_);
+ DCHECK(!initted_);
LoadFromPB(pb);
@@ -125,9 +125,8 @@ void RowSetMetadata::LoadFromPB(const RowSetDataPB& pb) {
}
void RowSetMetadata::ToProtobuf(RowSetDataPB *pb) {
- pb->set_id(id_);
-
std::lock_guard l(lock_);
+ pb->set_id(id_);
// Write Column Files
for (const ColumnIdToBlockIdMap::value_type& e : blocks_by_col_id_) {
@@ -175,7 +174,12 @@ void RowSetMetadata::ToProtobuf(RowSetDataPB *pb) {
}
const std::string RowSetMetadata::ToString() const {
- return Substitute("RowSet($0)", id_);
+ int64_t id = 0;
+ {
+ std::lock_guard l(lock_);
+ id = id_;
+ }
+ return Substitute("RowSet($0)", id);
}
void RowSetMetadata::SetColumnDataBlocks(const std::map<ColumnId, BlockId>&
blocks_by_col_id) {
diff --git a/src/kudu/tablet/rowset_metadata.h
b/src/kudu/tablet/rowset_metadata.h
index 4e33cfb0f..922270cac 100644
--- a/src/kudu/tablet/rowset_metadata.h
+++ b/src/kudu/tablet/rowset_metadata.h
@@ -97,7 +97,10 @@ class RowSetMetadata {
const std::string ToString() const;
- int64_t id() const { return id_; }
+ int64_t id() const {
+ std::lock_guard l(lock_);
+ return id_;
+ }
const SchemaPtr tablet_schema() const {
return tablet_metadata_->schema();
@@ -266,11 +269,14 @@ class RowSetMetadata {
TabletMetadata* const tablet_metadata_;
bool initted_;
- int64_t id_;
// Protects the below mutable fields.
mutable LockType lock_;
+ // Rowset identifier. Set in one of the constructors, and may be also set
+ // via LoadFromPB().
+ int64_t id_;
+
// The min and max keys of the rowset.
std::optional<std::string> min_encoded_key_;
std::optional<std::string> max_encoded_key_;
