Use FileChecksumGenFactory for SST file checksum #6600

zhichao-cao · 2020-03-26T17:53:19Z

In the current implementation, sst file checksum is calculated by a shared checksum function object, which may make some checksum function hard to be applied here such as SHA1. In this implementation, each sst file will have its own checksum generator obejct, created by FileChecksumGenFactory. User needs to implement its own FilechecksumGenerator and Factory to plugin the in checksum calculation method.

Test plan: tested with make asan_check

siying

We need to update HISTORY.md for public interface change.

siying · 2020-03-26T20:51:00Z

include/rocksdb/file_checksum.h

-  // Return a processed value of the checksum for store in somewhere
-  virtual std::string ProcessChecksum(const std::string& checksum) = 0;
+  // Get the checksum
+  virtual std::string GetChecksum() = 0;

  // Returns a name that identifies the current file checksum function.
  virtual const char* Name() const = 0;


Do we need this? Or is it enough to only have it in FileChecksumGenFactory?

Do we need this? Or is it enough to only have it in FileChecksumGenFactory?

It might be more clear to user different name for FilechecksumGenerator and the Factory. User can decide to use the same name or not I think.

Is it used in code for now? Where do you plan to use this Name()?

Is it used in code for now? Where do you plan to use this Name()?

Each time, after the SST file is generated, table builder will call to get the checksum generator name and checksum. The name of ChecksumGenFactory is not used yet.

siying · 2020-03-26T20:54:59Z

util/file_checksum_helper.h

    assert(data != nullptr);
-    return Uint32ToString(crc32c::Value(data, n));
+    if (is_inintilized_ == false) {
+      checksum_ = crc32c::Value(data, n);


Here is the implementation of crc32c::Value():

inline uint32_t Value(const char* data, size_t n) { return Extend(0, data, n); }

I don't think we need this if and is_inintilized_.

@siying Thanks for pointing out. I will change accordingly.

siying · 2020-03-26T20:55:18Z

util/file_checksum_helper.h

+ private:
+  uint32_t checksum_;
+  bool is_inintilized_;
+  std::string file_name_;


What is it used for?

@siying Will remove.

zhichao-cao · 2020-03-26T21:31:13Z

We need to update HISTORY.md for public interface change.

Sure.

riversand963 · 2020-03-26T21:54:12Z

include/rocksdb/file_checksum.h


  // Returns a name that identifies the current file checksum function.
  virtual const char* Name() const = 0;
 };

+// Create the FileChecksumGenerator object for each SST file.
+class FileChecksumGenFactory {
+ public:


Nit: explicitly write the default constructor.

include/rocksdb/file_checksum.h

file/writable_file_writer.h

siying · 2020-03-27T00:54:36Z

file/writable_file_writer.cc

+  if (checksum_generator_ != nullptr) {
+    return checksum_generator_->GetChecksum();
+  } else {
+    return kUnknownFileChecksum;


I guess it's not a part of the PR but does it make more sense to call it kNoFileChecksum? Similarly for kUnknownFileChecksumFuncName

@siying So change to kNoFileChecksum and kNoFileChecksumGenName ?

@siying. I will change them in the next PR.

siying · 2020-03-27T00:55:27Z

include/rocksdb/file_checksum.h

-  // Return a processed value of the checksum for store in somewhere
-  virtual std::string ProcessChecksum(const std::string& checksum) = 0;
+  // Get the checksum
+  virtual std::string GetChecksum() = 0;


I thought you would add a Finalize() function?

@siying Yes. Just pushed the updates.

riversand963 · 2020-03-27T23:44:49Z

file/writable_file_writer.cc

@@ -216,9 +216,18 @@ Status WritableFileWriter::Flush() {
  return s;
 }

+std::string WritableFileWriter::GetFileChecksum() {
+  if (checksum_generator_ != nullptr) {
+    checksum_generator_->Finalize();


Finalize() will be called each time we call GetFileChecksum(). This does not sound intended usage pattern in which I expect Finalize() to be called only once.

Maybe considering adding a member bool finalized_?

@riversand963 Good suggestion! I have not considered this situation. PR is updated accordingly

riversand963 · 2020-03-28T01:00:09Z

util/file_checksum_helper.h

-    uint32_t checksum_value = StringToUint32(checksum);
-    return Uint32ToString(crc32c::Mask(checksum_value));
-  }
+  void Finalize() override { checksum_str_ = Uint32ToString(checksum_); }


We should avoid/disallow calling Finalize multiple times. Currently it's possible due to the implementation of GetFileChecksum().

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying

I don't have blocking comment for now. I'll defer to @riversand963 to approve.

siying · 2020-03-28T02:53:21Z

file/writable_file_writer.cc

+  if (checksum_generator_ != nullptr) {
+    return checksum_generator_->GetChecksum();
+  } else {
+    return kUnknownFileChecksum;


siying · 2020-03-28T02:57:49Z

file/writable_file_writer.cc

+std::string WritableFileWriter::GetFileChecksum() {
+  if (checksum_generator_ != nullptr) {
+    if (!checksum_finalized_) {
+      checksum_generator_->Finalize();


Only calling Finalize() inside GetFileChecksum() defeats the purpose of Finalize(). The application can do it if they want to. In order for Finalize() to make sense, we should call it when we close the file. Otherwise, there is no point of having such a function.

@siying I see the logic here. Will move to Close and update the GetChecksum order in builder.cc

facebook-github-bot · 2020-03-28T06:26:39Z

@zhichao-cao has updated the pull request. Re-import the pull request

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying

The change looks good to me. Defer to @riversand963 to approve.

db/builder.cc

riversand963

LGTM. Thanks @zhichao-cao for working on this.

db/builder.cc

file/writable_file_writer.cc

zhichao-cao · 2020-03-29T07:23:42Z

LGTM. Thanks @zhichao-cao for working on this.

Thanks for the review and comments! Will change accordingly.

zhichao-cao · 2020-03-29T07:25:10Z

The change looks good to me. Defer to @riversand963 to approve.

Thanks for the review and comments!

facebook-github-bot · 2020-03-29T19:33:12Z

@zhichao-cao has updated the pull request. Re-import the pull request

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-03-31T09:35:02Z

@zhichao-cao merged this pull request in e8d332d.

Summary: In the current implementation, sst file checksum is calculated by a shared checksum function object, which may make some checksum function hard to be applied here such as SHA1. In this implementation, each sst file will have its own checksum generator obejct, created by FileChecksumGenFactory. User needs to implement its own FilechecksumGenerator and Factory to plugin the in checksum calculation method. Pull Request resolved: facebook/rocksdb#6600 Test Plan: tested with make asan_check Reviewed By: riversand963 Differential Revision: D20717670 Pulled By: zhichao-cao fbshipit-source-id: 2a74c1c280ac11a07a1980185b43b671acaa71c6 Signed-off-by: Changlong Chen <levisonchen@live.cn>

zhichao-cao requested review from siying and riversand963 March 26, 2020 17:53

facebook-github-bot added the CLA Signed label Mar 26, 2020

siying reviewed Mar 26, 2020

View reviewed changes

riversand963 reviewed Mar 26, 2020

View reviewed changes

siying reviewed Mar 27, 2020

View reviewed changes

file/writable_file_writer.h Outdated Show resolved Hide resolved

siying reviewed Mar 27, 2020

View reviewed changes

zhichao-cao force-pushed the checksum_factory branch from 41179a7 to afa8f0e Compare March 27, 2020 00:58

riversand963 reviewed Mar 28, 2020

View reviewed changes

zhichao-cao force-pushed the checksum_factory branch from 78413ac to fa33d35 Compare March 28, 2020 02:06

facebook-github-bot reviewed Mar 28, 2020

View reviewed changes

siying reviewed Mar 28, 2020

View reviewed changes

facebook-github-bot reviewed Mar 28, 2020

View reviewed changes

siying reviewed Mar 29, 2020

View reviewed changes

db/builder.cc Outdated Show resolved Hide resolved

riversand963 approved these changes Mar 29, 2020

View reviewed changes

db/builder.cc Outdated Show resolved Hide resolved

file/writable_file_writer.cc Outdated Show resolved Hide resolved

zhichao-cao added 7 commits March 29, 2020 11:50

Use FileChecksumGenFactory for SST file checksum

4892af0

Review changes

f1e8e88

Address the review comments

b16c90c

Change ChecksumGenOptions to ChecksumGenContext

064fa7e

Add the finalize flag to writable_file_writer

ab49b5a

Adjust the finalize order

9f84600

Adjust the code

6934013

zhichao-cao force-pushed the checksum_factory branch from d58e66e to 6934013 Compare March 29, 2020 19:32

facebook-github-bot reviewed Mar 29, 2020

View reviewed changes

facebook-github-bot closed this in e8d332d Mar 29, 2020

facebook-github-bot added the Merged label Mar 31, 2020

Use FileChecksumGenFactory for SST file checksum #6600

Use FileChecksumGenFactory for SST file checksum #6600

Conversation

zhichao-cao commented Mar 26, 2020

siying left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siying Mar 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhichao-cao commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 28, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

zhichao-cao commented Mar 29, 2020

zhichao-cao commented Mar 29, 2020

facebook-github-bot commented Mar 29, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 31, 2020

siying Mar 27, 2020 •

edited