-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
libroach: make DBSstFileWriter output work with time bound iterator #27966
Conversation
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 it would be cleaner to move the properties collector into its own header file (tableprops.h?), but totally fine as is!
Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)
c-deps/libroach/db.cc, line 712 at r1 (raw file):
std::shared_ptr<rocksdb::TablePropertiesCollectorFactory> time_bound_prop_collector( DBMakeTimeBoundCollector()); options->table_properties_collector_factories.push_back(time_bound_prop_collector);
You should be able to either do
options->table_properties_collector_factories.emplace_back(DBMakeTimeBoundCollector());
or
options->table_properties_collector_factories.emplace_back(
std::make_shared(DBMakeTimeBoundCollector()));
to avoid the temporary.
4f34705
to
158d456
Compare
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.
Moved into timebound.{h,cc}. RFAL!
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)
c-deps/libroach/db.cc, line 712 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
You should be able to either do
options->table_properties_collector_factories.emplace_back(DBMakeTimeBoundCollector());
or
options->table_properties_collector_factories.emplace_back( std::make_shared(DBMakeTimeBoundCollector()));
to avoid the temporary.
Done
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.
Reviewed 8 of 8 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)
The time bounded iterator optimization looks for sstable metadata that indicates the min and max mvcc timestamp included. The iterator (which is configured with a min and max bounds) can use this to skip tables that don't have relevant data. When the bounds aren't set, it has to fall back to checking the table. The TimeBoundTblPropCollectorFactory already sets the metadata on sstable creation, but it wasn't hooked up to DBSstFileWriter. This meant that IMPORT and RESTORE (both of which use DBSstFileWriter via the Import command) would create and ingest sstables that hit the no bounds fall back. This commit adds TimeBoundTblPropCollectorFactory to DBSstFileWriter which means data ingested via RESTORE and IMPORT now work with time bounded iterators just like any other sstables. Closes cockroachdb#26870 Release note (performance improvement): data ingested with RESTORE and IMPORT is now eligible for a performance optimization used in incremental BACKUP and CHANGEFEEDs
Thanks for the reviews! bors r=benesch |
27966: libroach: make DBSstFileWriter output work with time bound iterator r=benesch a=danhhz The time bounded iterator optimization looks for sstable metadata that indicates the min and max mvcc timestamp included. The iterator (which is configured with a min and max bounds) can use this to skip tables that don't have relevant data. When the bounds aren't set, it has to fall back to checking the table. The TimeBoundTblPropCollectorFactory already sets the metadata on sstable creation, but it wasn't hooked up to DBSstFileWriter. This meant that IMPORT and RESTORE (both of which use DBSstFileWriter via the Import command) would create and ingest sstables that hit the no bounds fall back. This commit adds TimeBoundTblPropCollectorFactory to DBSstFileWriter which means data ingested via RESTORE and IMPORT now work with time bounded iterators just like any other sstables. Closes #26870 Release note (performance improvement): data ingested with RESTORE and IMPORT is now eligible for a performance optimization used in incremental BACKUP and CHANGEFEEDs Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
Build succeeded |
The time bounded iterator optimization looks for sstable metadata that
indicates the min and max mvcc timestamp included. The iterator (which
is configured with a min and max bounds) can use this to skip tables
that don't have relevant data. When the bounds aren't set, it has to
fall back to checking the table.
The TimeBoundTblPropCollectorFactory already sets the metadata on
sstable creation, but it wasn't hooked up to DBSstFileWriter. This meant
that IMPORT and RESTORE (both of which use DBSstFileWriter via the
Import command) would create and ingest sstables that hit the no bounds
fall back.
This commit adds TimeBoundTblPropCollectorFactory to DBSstFileWriter
which means data ingested via RESTORE and IMPORT now work with time
bounded iterators just like any other sstables.
Closes #26870
Release note (performance improvement): data ingested with RESTORE and
IMPORT is now eligible for a performance optimization used in
incremental BACKUP and CHANGEFEEDs