fix bug about NewCompactOnDeletionCollectorFactory hasn't effect on L0 flush

[yuwenlong123]

Because in BuildTable function, builder->Finish() will reset need_comapct_ to false before builder->NeedCompact assigns it to marked_for_compaction.

    s = c_iter.status();
    if (!s.ok() || empty) {
      builder->Abandon();
    } else {
      s = builder->Finish();
    }   

    if (s.ok() && !empty) {
      uint64_t file_size = builder->FileSize();
      meta->fd.file_size = file_size;
      meta->marked_for_compaction = builder->NeedCompact;
      assert(meta->fd.GetFileSize() > 0); 
      tp = builder->GetTableProperties(); // refresh now that builder is finished
      if (table_properties) {
        *table_properties = tp; 
      }   
    }

[maysamyabandeh]

There is no variable need_comapct_ in the code base. Is there any unit test that would reproduce this?

[yuwenlong123]

“There is no variable need_comapct_ in the code base”，you can review this code, "builder->Finish()" as below stack.

#0 rocksdb::CompactOnDeletionCollector::Reset (this=0x28a6d00) at utilities/table_properties_collectors/compact_on_deletion_collector.cc:27
#1 0x0000000000bb5034 in rocksdb::CompactOnDeletionCollector::Finish (this=, properties=)
at ./utilities/table_properties_collectors/compact_on_deletion_collector.h:64
#2 0x0000000000c06656 in rocksdb::UserKeyTablePropertiesCollector::Finish (this=, properties=)
at db/table_properties_collector.cc:107
#3 0x0000000000b7fe8d in rocksdb::NotifyCollectTableCollectorsOnFinish (collectors=..., info_log=0x27d2140, builder=builder@entry=0x7f28c64a8590)
at table/meta_blocks.cc:147
#4 0x0000000000c1b891 in rocksdb::BlockBasedTableBuilder::Finish (this=0x2fab2e0) at table/block_based_table_builder.cc:749
#5 0x0000000000bd9d43 in rocksdb::BuildTable (dbname=..., env=0x1772220 rocksdb::Env::Default()::default_env, ioptions=..., mutable_cf_options=...,
env_options=..., table_cache=0x285db80, iter=0x7f28c64a9548, range_del_iter=..., meta=0x7f28c64aa5b8, internal_comparator=...,
int_tbl_prop_collector_factories=0x2830470, column_family_id=2, column_family_name=..., snapshots=...,
earliest_write_conflict_snapshot=72057594037927935, compression=, compression_opts=..., paranoid_file_checks=false,
internal_stats=0x2840500, reason=rocksdb::TableFileCreationReason::kFlush, hotcache_version=0, event_logger=0x27cd940, job_id=268,
io_priority=rocksdb::Env::IO_HIGH, table_properties=0x7f28c64aa3d8, level=0, creation_time=1565159117) at db/builder.cc:170
#6 0x0000000000ac60ec in rocksdb::FlushJob::WriteLevel0Table (this=this@entry=0x7f28c64aa340) at db/flush_job.cc:306
#7 0x0000000000ac8572 in rocksdb::FlushJob::Run (this=this@entry=0x7f28c64aa340, file_meta=file_meta@entry=0x7f28c64aa0b0) at db/flush_job.cc:183
#8 0x0000000000a8c8e5 in rocksdb::DBImpl::FlushMemTableToOutputFile (this=this@entry=0x27ccd00, cfd=cfd@entry=0x2830400, mutable_cf_options=...,
made_progress=made_progress@entry=0x7f28c64aac8f, job_context=job_context@entry=0x7f28c64aacc0, log_buffer=0x7f28c64aaec0)
at db/db_impl_compaction_flush.cc:126
#9 0x0000000000a8d418 in rocksdb::DBImpl::BackgroundFlush (this=this@entry=0x27ccd00, made_progress=made_progress@entry=0x7f28c64aac8f,
job_context=job_context@entry=0x7f28c64aacc0, log_buffer=log_buffer@entry=0x7f28c64aaec0) at db/db_impl_compaction_flush.cc:1234
#10 0x0000000000a917ce in rocksdb::DBImpl::BackgroundCallFlush (this=0x27ccd00) at db/db_impl_compaction_flush.cc:1259
#11 0x0000000000baf54c in std::function<void ()>::operator()() const (this=0x7f28c64ab830) at /usr/local/include/c++/5.5.0/functional:2267
#12 rocksdb::ThreadPoolImpl::Impl::BGThread (this=this@entry=0x25381c0, thread_id=thread_id@entry=0) at util/threadpool_imp.cc:237
#13 0x0000000000baf71d in rocksdb::ThreadPoolImpl::Impl::BGThreadWrapper (arg=0x237e7e0) at util/threadpool_imp.cc:261
#14 0x00000000010e3380 in std::execute_native_thread_routine (__p=) at ../../../../../gcc-5.5.0/libstdc++-v3/src/c++11/thread.cc:84
#15 0x00007f28ca1a4dc5 in start_thread () from /lib64/libpthread.so.0
#16 0x00007f28c95aa21d in clone () from /lib64/libc.so.6

and then

27 void CompactOnDeletionCollector::Reset() {
28 for (int i = 0; i < kNumBuckets; ++i) {
29 num_deletions_in_buckets_[i] = 0;
30 }
31 current_bucket_ = 0;
(gdb)
32 num_keys_in_current_bucket_ = 0;
33 num_deletions_in_observation_window_ = 0;
34 need_compaction_ = false; // this is key code.
35 }

so, as f5 BuildTable function describe, it can be simplify as below.

for (; c_iter.Valid(); c_iter.Next()) {
builder->Add(key, value); //maybe set need_compaction_ = true
}
s = builder->Finish(); // always set need_compaction_ = false
meta->marked_for_compaction = builder->NeedCompact(); // it can't ture for ever

Finally, meta->marked_for_compaction is always false.

[miasantreble]

would it be possible to reproduce the issue in a unit test?

[yuwenlong123]

@miasantreble
you can set options.table_properties_collector_factories.push_back(rocksdb::NewCompactOnDeletionCollectorFactory(1000, 1));
(1000, 1) means trigger this delete collector surely make "need_compaction_ = true", then put and delete some random key, then check the log in flush_job.cc,

ROCKS_LOG_INFO(db_options_.info_log,
"[%s] [JOB %d] Level-0 flush table #%" PRIu64 ": %" PRIu64
" bytes %s"
"%s",
cfd_->GetName().c_str(), job_context_->job_id,
meta_.fd.GetNumber(), meta_.fd.GetFileSize(),
s.ToString().c_str(),
meta_.marked_for_compaction ? " (needs compaction)" : "")

you will see meta_.marked_for_compaction is always false, that "(needs compaction)" never never print.

[miasantreble]

if so can you please add a unit test for the bug? thanks

[maysamyabandeh]

Closing it since it was not reproduced,

[siying]

@yuwenlong123 which version of RocksDB are you running on. It appears that CompactOnDeletionCollector::Finish() stopped calling CompactOnDeletionCollector::Reset() since two years ago: #2936 . Can you try out the latest release and see whether the bug is still there?

[yuwenlong123]

@siying sorry about that, my rocksdb version is 5.7.2 which is before #2936. Then, I have check the latest code. There is no such problem. Thanks for your reply.