This is an automated email from the ASF dual-hosted git repository.
achennaka 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 63dcd5698 [tablet] modernize MultiColumnWriter's code
63dcd5698 is described below
commit 63dcd56985516e554e8f00474b070b318a598008
Author: Alexey Serbin <ale...@apache.org>
AuthorDate: Mon Nov 13 21:40:01 2023 -0800
[tablet] modernize MultiColumnWriter's code
This patch contain a few micro-optimizations, but it doesn't contain
any functional changes:
* std::unique_ptr replaced raw pointers in the cfile_writers_ array,
so no need to use the STLDeleteElements macro
* replaced CHECK() with DCHECK() where appropriate
* added more DCHECK() macros to enforce logical consistency
* removed virtual base from the class and made it final
* added tablet identifier into status objects and error messages
to make it possible to attribute errors to particular tablets
Change-Id: Ife716bf62338ca896072ce7fce3aea9d5f5204ca
Reviewed-on: http://gerrit.cloudera.org:8080/20702
Tested-by: Alexey Serbin <ale...@apache.org>
Tested-by: Kudu Jenkins
Reviewed-by: Abhishek Chennaka <achenn...@cloudera.com>
---
src/kudu/tablet/multi_column_writer.cc | 98 ++++++++++++++++++----------------
src/kudu/tablet/multi_column_writer.h | 26 ++++-----
2 files changed, 62 insertions(+), 62 deletions(-)
diff --git a/src/kudu/tablet/multi_column_writer.cc
b/src/kudu/tablet/multi_column_writer.cc
index 04efecb9f..1c12b6490 100644
--- a/src/kudu/tablet/multi_column_writer.cc
+++ b/src/kudu/tablet/multi_column_writer.cc
@@ -30,38 +30,41 @@
#include "kudu/fs/block_id.h"
#include "kudu/fs/block_manager.h"
#include "kudu/fs/fs_manager.h"
-#include "kudu/gutil/stl_util.h"
+#include "kudu/gutil/port.h"
#include "kudu/gutil/strings/substitute.h"
+using kudu::cfile::CFileWriter;
+using kudu::fs::BlockCreationTransaction;
+using kudu::fs::CreateBlockOptions;
+using kudu::fs::WritableBlock;
+using std::map;
+using std::string;
+using std::unique_ptr;
+using strings::Substitute;
+
namespace kudu {
namespace tablet {
-using cfile::CFileWriter;
-using fs::BlockCreationTransaction;
-using fs::CreateBlockOptions;
-using fs::WritableBlock;
-using std::unique_ptr;
-
MultiColumnWriter::MultiColumnWriter(FsManager* fs,
const Schema* schema,
- std::string tablet_id)
- : fs_(fs),
- schema_(schema),
- finished_(false),
- tablet_id_(std::move(tablet_id)) {
-}
-
-MultiColumnWriter::~MultiColumnWriter() {
- STLDeleteElements(&cfile_writers_);
+ string tablet_id)
+ : fs_(fs),
+ schema_(DCHECK_NOTNULL(schema)),
+ tablet_id_(std::move(tablet_id)),
+ open_(false),
+ finished_(false) {
+ cfile_writers_.reserve(schema_->num_columns());
+ block_ids_.reserve(schema_->num_columns());
}
Status MultiColumnWriter::Open() {
- CHECK(cfile_writers_.empty());
+ DCHECK(!open_) << "already open";
+ DCHECK(cfile_writers_.empty()); // this method isn't re-entrant after
failures
// Open columns.
const CreateBlockOptions block_opts({ tablet_id_ });
- for (int i = 0; i < schema_->num_columns(); i++) {
- const ColumnSchema &col = schema_->column(i);
+ for (auto i = 0; i < schema_->num_columns(); ++i) {
+ const auto& col = schema_->column(i);
cfile::WriterOptions opts;
@@ -69,7 +72,7 @@ Status MultiColumnWriter::Open() {
// the corresponding rows.
opts.write_posidx = true;
- /// Set the column storage attributes.
+ // Set the column storage attributes.
opts.storage_attributes = col.attributes();
// If the schema has a single PK and this is the PK col
@@ -77,32 +80,32 @@ Status MultiColumnWriter::Open() {
opts.write_validx = true;
}
- // Open file for write.
+ // Open file for writing.
unique_ptr<WritableBlock> block;
RETURN_NOT_OK_PREPEND(fs_->CreateNewBlock(block_opts, &block),
- "Unable to open output file for column " +
col.ToString());
+ Substitute("tablet $0: unable to open output file for column $1",
+ tablet_id_, col.ToString()));
BlockId block_id(block->id());
// Create the CFile writer itself.
unique_ptr<CFileWriter> writer(new CFileWriter(
- std::move(opts),
- col.type_info(),
- col.is_nullable(),
- std::move(block)));
+ std::move(opts), col.type_info(), col.is_nullable(),
std::move(block)));
RETURN_NOT_OK_PREPEND(writer->Start(),
- "Unable to Start() writer for column " +
col.ToString());
-
- cfile_writers_.push_back(writer.release());
- block_ids_.push_back(block_id);
+ Substitute("tablet $0: unable to start writer for column $1",
+ tablet_id_, col.ToString()));
+ cfile_writers_.emplace_back(std::move(writer));
+ block_ids_.emplace_back(block_id);
}
- VLOG(1) << strings::Substitute("Opened CFile writers for $0 column(s)",
- cfile_writers_.size());
+ open_ = true;
+ VLOG(1) << Substitute("Opened CFile writers for $0 column(s)",
+ cfile_writers_.size());
return Status::OK();
}
Status MultiColumnWriter::AppendBlock(const RowBlock& block) {
- for (int i = 0; i < schema_->num_columns(); i++) {
+ DCHECK(open_);
+ for (auto i = 0; i < schema_->num_columns(); ++i) {
ColumnBlock column = block.column_block(i);
if (column.is_nullable()) {
RETURN_NOT_OK(cfile_writers_[i]->AppendNullableEntries(column.non_null_bitmap(),
@@ -116,13 +119,14 @@ Status MultiColumnWriter::AppendBlock(const RowBlock&
block) {
Status MultiColumnWriter::FinishAndReleaseBlocks(
BlockCreationTransaction* transaction) {
- CHECK(!finished_);
- for (int i = 0; i < schema_->num_columns(); i++) {
- CFileWriter *writer = cfile_writers_[i];
- Status s = writer->FinishAndReleaseBlock(transaction);
- if (!s.ok()) {
- LOG(WARNING) << "Unable to Finish writer for column " <<
- schema_->column(i).ToString() << ": " << s.ToString();
+ DCHECK(open_);
+ DCHECK(!finished_);
+ for (auto i = 0; i < schema_->num_columns(); ++i) {
+ auto s = cfile_writers_[i]->FinishAndReleaseBlock(transaction);
+ if (PREDICT_FALSE(!s.ok())) {
+ LOG(ERROR) << Substitute(
+ "tablet $0: unable to finialize writer for column $1",
+ tablet_id_, schema_->column(i).ToString());
return s;
}
}
@@ -130,17 +134,19 @@ Status MultiColumnWriter::FinishAndReleaseBlocks(
return Status::OK();
}
-void MultiColumnWriter::GetFlushedBlocksByColumnId(std::map<ColumnId,
BlockId>* ret) const {
- CHECK(finished_);
- ret->clear();
- for (int i = 0; i < schema_->num_columns(); i++) {
- (*ret)[schema_->column_id(i)] = block_ids_[i];
+void MultiColumnWriter::GetFlushedBlocksByColumnId(map<ColumnId, BlockId>*
ret) const {
+ DCHECK(finished_);
+ auto& r = *ret;
+ r.clear();
+ for (auto i = 0; i < schema_->num_columns(); ++i) {
+ r[schema_->column_id(i)] = block_ids_[i];
}
}
size_t MultiColumnWriter::written_size() const {
+ DCHECK(open_);
size_t size = 0;
- for (const CFileWriter *writer : cfile_writers_) {
+ for (const auto& writer: cfile_writers_) {
size += writer->written_size();
}
return size;
diff --git a/src/kudu/tablet/multi_column_writer.h
b/src/kudu/tablet/multi_column_writer.h
index 259d0bc61..1538035cd 100644
--- a/src/kudu/tablet/multi_column_writer.h
+++ b/src/kudu/tablet/multi_column_writer.h
@@ -14,16 +14,16 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
-#ifndef KUDU_TABLET_MULTI_COLUMN_WRITER_H
-#define KUDU_TABLET_MULTI_COLUMN_WRITER_H
#include <cstddef>
#include <map>
+#include <memory>
#include <string>
#include <vector>
#include <glog/logging.h>
+#include "kudu/cfile/cfile_writer.h"
#include "kudu/fs/block_id.h"
#include "kudu/gutil/macros.h"
#include "kudu/util/status.h"
@@ -35,10 +35,6 @@ class RowBlock;
class Schema;
struct ColumnId;
-namespace cfile {
-class CFileWriter;
-} // namespace cfile
-
namespace fs {
class BlockCreationTransaction;
} // namespace fs
@@ -47,13 +43,13 @@ namespace tablet {
// Wrapper which writes several columns in parallel corresponding to some
// Schema. Written blocks will fall in the tablet_id's data dir group.
-class MultiColumnWriter {
+class MultiColumnWriter final {
public:
MultiColumnWriter(FsManager* fs,
const Schema* schema,
std::string tablet_id);
- virtual ~MultiColumnWriter();
+ ~MultiColumnWriter() = default;
// Open and start writing the columns.
Status Open();
@@ -70,9 +66,9 @@ class MultiColumnWriter {
// Return the number of bytes written so far.
size_t written_size() const;
- cfile::CFileWriter* writer_for_col_idx(int i) {
- DCHECK_LT(i, cfile_writers_.size());
- return cfile_writers_[i];
+ cfile::CFileWriter* writer_for_col_idx(size_t i) {
+ CHECK_LT(i, cfile_writers_.size());
+ return cfile_writers_[i].get();
}
// Return the block IDs of the written columns, keyed by column ID.
@@ -83,17 +79,15 @@ class MultiColumnWriter {
private:
FsManager* const fs_;
const Schema* const schema_;
-
- bool finished_;
-
const std::string tablet_id_;
- std::vector<cfile::CFileWriter *> cfile_writers_;
+ std::vector<std::unique_ptr<cfile::CFileWriter>> cfile_writers_;
std::vector<BlockId> block_ids_;
+ bool open_;
+ bool finished_;
DISALLOW_COPY_AND_ASSIGN(MultiColumnWriter);
};
} // namespace tablet
} // namespace kudu
-#endif /* KUDU_TABLET_MULTI_COLUMN_WRITER_H */
