-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement BetaRowsetWriter #1590
Conversation
…loc exception in AggregateFuncTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -56,19 +56,13 @@ class AlphaRowsetWriter : public RowsetWriter { | |||
// get a rowset | |||
RowsetSharedPtr build() override; | |||
|
|||
MemPool* mem_pool() override; | |||
Version version() override { return _rowset_writer_context.version; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add const description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will fix later
|
||
RowsetSharedPtr build() override; | ||
|
||
Version version() override { return _context.version; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
_rowset_meta)); | ||
auto status = rowset->init(); | ||
if (status != OLAP_SUCCESS) { | ||
LOG(WARNING) << "rowset init failed when build new rowset"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add status msg to log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because OLAPStatus is just an enum and doesn't have errmsg field, the real reason is usually not logged by caller, but by the function who encounters error in the first place
OLAPStatus BetaRowsetWriter::_create_segment_writer() { | ||
auto path = BetaRowset::segment_file_path(_context.rowset_path_prefix, _context.rowset_id, _num_segment); | ||
segment_v2::SegmentWriterOptions writer_options; | ||
_segment_writer.reset(new segment_v2::SegmentWriter(path, _num_segment, _context.tablet_schema, writer_options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SegmentWriter can add another constructor without SegmentWriterOptions as a argument, and use the default SegmentWriterOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it can be optimized in the future
BetaRowsetWriter is used to write rowset in V2 segment format.
This PR contains several interface changes
link_files_to
because hard links are also useful in copy task, linked schema change, etccopy_files_to
to be consistent with other namesFuture works
write_mbytes_per_sec
based on writer type (load/base compaction/cumulative compaction)