curvefs/metaserver: fixed rocksdb storage memory leak caused by unrel… #1388
Conversation
|
recheck |
| @@ -355,6 +361,10 @@ class RocksDBStorageIterator : public Iterator { | |||
| storage_->db_->ReleaseSnapshot(readOptions_.snapshot); | |||
| } | |||
| } | |||
|
|
|||
| if (iter_ != nullptr) { | |||
There was a problem hiding this comment.
wrap iter_ with a std::unique_ptr?
There was a problem hiding this comment.
wrap
iter_with a std::unique_ptr?
fixed.
| @@ -86,12 +86,14 @@ class RocksDBOptions { | |||
| std::vector<ColumnFamilyDescriptor> columnFamilies_; | |||
|
|
|||
| static const std::string kOrderedColumnFamilyName_; | |||
|
|
|||
| std::shared_ptr<rocksdb::Comparator> comparator_; | |||
There was a problem hiding this comment.
there is no need to wrap it with a std::shared_ptr, just storing the raw value is enough.
There was a problem hiding this comment.
If we storing raw value, like rocksdb::Comparator*, the memory of comparator object will never be freed:
comparator_ = std::make_shared<RocksDBStorageComparator>();
...
cfOptions.comparator = comparator_.get();
| auto unorderedCFOptions = ColumnFamilyOptions(); | ||
| auto orderedCFOptions = ColumnFamilyOptions(); | ||
| comparator_ = std::make_shared<RocksDBStorageComparator>(); | ||
| auto cfOptions = ColumnFamilyOptions(); |
There was a problem hiding this comment.
ColumnFamilyOptions cfOptions;
There was a problem hiding this comment.
ColumnFamilyOptions cfOptions;
fixed.
| return Status::NotSupported(); | ||
| } | ||
| pending4set_.clear(); | ||
| pending4del_.clear(); | ||
| return ToStorageStatus(txn_->Rollback()); | ||
| Status s = ToStorageStatus(txn_->Commit()); | ||
| delete txn_; |
There was a problem hiding this comment.
ditto, use std::unique_ptr if possiable
There was a problem hiding this comment.
agree
the txn_ is the member of RocksDBStorage, we should free it manually when transaction finished, the std::unique_ptr is not work.
| cfOptions.enable_blob_files = true; | ||
| cfOptions.level_compaction_dynamic_level_bytes = true; | ||
| cfOptions.compaction_pri = ROCKSDB_NAMESPACE::kMinOverlappingRatio; | ||
| cfOptions.prefix_extractor.reset(NewCappedPrefixTransform(3)); |
There was a problem hiding this comment.
Does some options here important and add some comments？
There was a problem hiding this comment.
Does some options here important and add some comments？
fixed.
| return Status::NotSupported(); | ||
| } | ||
| pending4set_.clear(); | ||
| pending4del_.clear(); | ||
| return ToStorageStatus(txn_->Rollback()); | ||
| Status s = ToStorageStatus(txn_->Commit()); | ||
| delete txn_; |
…easing iterator and transaction. (#1381)
…easing iterator and transaction.
What problem does this PR solve?
Issue Number: #1381
Problem Summary: rocksdb storage memory leak
What is changed and how it works?
What's Changed:
How it Works:
Side effects(Breaking backward compatibility? Performance regression?):
Check List