[Skinny] Add AppendWithVerify and PositionedAppendWithVerify to Env and FileSystem #7419

zhichao-cao · 2020-09-21T23:16:38Z

Add new AppendWithVerify and PositionedAppendWithVerify APIs to Env and FileSystem to bring the data verification information (data checksum information) from upper layer (e.g., WritableFileWriter) to the storage layer. This PR only include the API definition, no functional codes are added to unblock other developers which depend on these APIs.

Test plan: make -j32

include/rocksdb/env.h

siying · 2020-09-21T23:22:41Z

include/rocksdb/env.h

+struct DataVerificationInfo {
+  DataVerificationInfo(const std::string& checksum_) : checksum(checksum_) {}
+  // checksum of the data being written.
+  Slice checksum;


I don't have a strong preference but think about pros and cons between const Slice& and Slice.

What I'm thinking is the member of DataVerificationInfo can be changed (reuse of the variable). If the struct member is const, we are not able to change it once the DataVerificationInfo variable is constructed. And yes, the constructor is not necessary here.

include/rocksdb/file_system.h

include/rocksdb/env.h

include/rocksdb/file_system.h

pdillinger · 2020-09-22T15:52:39Z

include/rocksdb/file_system.h

@@ -749,13 +756,33 @@ class FSWritableFile {
  //
  // PositionedAppend() requires aligned buffer to be passed in. The alignment
  // required is queried via GetRequiredBufferAlignment()
+


Looks like this should be moved above the PositionedAppend comments above.

pdillinger · 2020-09-22T16:16:13Z

include/rocksdb/file_system.h

+  // the future. Currently, RocksDB does not use this API.
+  virtual IOStatus Append(const Slice& data, const IOOptions& options,
+                          IODebugContext* dbg,
+                          const DataVerificationInfo& /* verification_info */) {


I seems a little odd to me for DataVerificationInfo to come after IOOptions and IODebugContext. To me, this information seems more "important" than IOOptions and IODebugContext so should come before them. Of course DataVerificationInfo is less "important" than data (and offset). For example, the parameter orderings would be weird to me if we added another parameter onto the end of both Append overloads.

What do others think?

I agree that the order is suboptimal. dbg does feel should come later than DataVerificationInfo. Not sure IOOptions. RocksDB public functions tend to put options before data arguments, e.g. DB::Get() and DB::Put().

Yes, it make more scene. How about between IOOotions and IOdebugContext? @pdillinger @siying

pdillinger · 2020-09-22T17:54:11Z

I also think we should mark these as EXPERIMENTAL / CURRENTLY UNUSED in file_system.h

mrambacher

I still feel strongly that the DataVerification does not belong as an argument to the API but should instead by part of the IOOptions. The comment in IOOptions say these are "hints to each request that the FileSystem may or may not honor". When you add new APIs, you are implying that the FileSystem will honor the arguments.

How does a caller know if DataVerification will be honored? It is proper for a FileSystem to just ignore that argument or not? If a FileSystem can ignore the argument, how is it different than the values in IOOptions? What happens if a Verification is passed in that is bad -- will the caller get an error always, sometimes, or never?

The IOOptions already exactly matches the functionality you are requesting. The comment says "FileSystems may or may not honor these", which applies here. There are no APIs to change and compatibility is preserved. If no value is specified, things will work exactly as expected. If one is specified, a FileSystem is free to check it and return an error (and it would be easy to write a FileSystem that did exactly that which wrapped another FileSystem).

Adding it to the IOOptions simplifies testing, documentation, and minimizes code changes. It seems like a much cleaner solution to me.

…stem

zhichao-cao · 2020-09-23T21:52:43Z

I still feel strongly that the DataVerification does not belong as an argument to the API but should instead by part of the IOOptions. The comment in IOOptions say these are "hints to each request that the FileSystem may or may not honor". When you add new APIs, you are implying that the FileSystem will honor the arguments.

How does a caller know if DataVerification will be honored? It is proper for a FileSystem to just ignore that argument or not? If a FileSystem can ignore the argument, how is it different than the values in IOOptions? What happens if a Verification is passed in that is bad -- will the caller get an error always, sometimes, or never?

The IOOptions already exactly matches the functionality you are requesting. The comment says "FileSystems may or may not honor these", which applies here. There are no APIs to change and compatibility is preserved. If no value is specified, things will work exactly as expected. If one is specified, a FileSystem is free to check it and return an error (and it would be easy to write a FileSystem that did exactly that which wrapped another FileSystem).

Adding it to the IOOptions simplifies testing, documentation, and minimizes code changes. It seems like a much cleaner solution to me.

Based on several discussion, it would be better to explicitly use DataVerificationInfo as input parameter instead of "hide" it in the IOOptions. Since it is still experimental, we may change that in the future based on the use cases.

facebook-github-bot

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

anand1976 · 2020-09-23T22:56:24Z

env/file_system_tracer.h

+  IOStatus Append(const Slice& data, const IOOptions& options,
+                  const DataVerificationInfo& /*verification_info*/,
+                  IODebugContext* dbg) override {
+    return Append(data, options, dbg);


This should be implemented in file_system_tracer.cc rather than redirecting here.

anand1976 · 2020-09-23T22:56:40Z

env/file_system_tracer.h

+                            const IOOptions& options,
+                            const DataVerificationInfo& /*verification_info*/,
+                            IODebugContext* dbg) override {
+    return PositionedAppend(data, offset, options, dbg);


Same comment as above

pdillinger

Anand said in chat that his concerns could be addressed in follow-up.

LGTM for experimental API to unblock internal dependency

facebook-github-bot · 2020-09-24T02:37:25Z

@zhichao-cao merged this pull request in 0ce9b3a.

…stem (#7419) Summary: Add new AppendWithVerify and PositionedAppendWithVerify APIs to Env and FileSystem to bring the data verification information (data checksum information) from upper layer (e.g., WritableFileWriter) to the storage layer. This PR only include the API definition, no functional codes are added to unblock other developers which depend on these APIs. Pull Request resolved: #7419 Test Plan: make -j32 Reviewed By: pdillinger Differential Revision: D23883196 Pulled By: zhichao-cao fbshipit-source-id: 94676c26bc56144cc32e3661f84f21eccd790411

Summary: in PR #7419 , we introduce the new Append and PositionedAppend APIs to WritableFile at File System, which enable RocksDB to pass the data verification information (e.g., checksum of the data) to the lower layer. In this PR, we use the new API in WritableFileWriter, such that the file created via WritableFileWrite can pass the checksum to the storage layer. To control which types file should apply the checksum handoff, we add checksum_handoff_file_types to DBOptions. User can use this option to control which file types (Currently supported file tyes: kLogFile, kTableFile, kDescriptorFile.) should use the new Append and PositionedAppend APIs to handoff the verification information. Pull Request resolved: #7523 Test Plan: add new unit test, pass make check/ make asan_check Reviewed By: pdillinger Differential Revision: D24313271 Pulled By: zhichao-cao fbshipit-source-id: aafd69091ae85c3318e3e17cbb96fe7338da11d0

…stem (facebook#7419) Summary: Add new AppendWithVerify and PositionedAppendWithVerify APIs to Env and FileSystem to bring the data verification information (data checksum information) from upper layer (e.g., WritableFileWriter) to the storage layer. This PR only include the API definition, no functional codes are added to unblock other developers which depend on these APIs. Pull Request resolved: facebook#7419 Test Plan: make -j32 Reviewed By: pdillinger Differential Revision: D23883196 Pulled By: zhichao-cao fbshipit-source-id: 94676c26bc56144cc32e3661f84f21eccd790411

Summary: in PR facebook#7419 , we introduce the new Append and PositionedAppend APIs to WritableFile at File System, which enable RocksDB to pass the data verification information (e.g., checksum of the data) to the lower layer. In this PR, we use the new API in WritableFileWriter, such that the file created via WritableFileWrite can pass the checksum to the storage layer. To control which types file should apply the checksum handoff, we add checksum_handoff_file_types to DBOptions. User can use this option to control which file types (Currently supported file tyes: kLogFile, kTableFile, kDescriptorFile.) should use the new Append and PositionedAppend APIs to handoff the verification information. Pull Request resolved: facebook#7523 Test Plan: add new unit test, pass make check/ make asan_check Reviewed By: pdillinger Differential Revision: D24313271 Pulled By: zhichao-cao fbshipit-source-id: aafd69091ae85c3318e3e17cbb96fe7338da11d0

…stem (#7419) Summary: Add new AppendWithVerify and PositionedAppendWithVerify APIs to Env and FileSystem to bring the data verification information (data checksum information) from upper layer (e.g., WritableFileWriter) to the storage layer. This PR only include the API definition, no functional codes are added to unblock other developers which depend on these APIs. Pull Request resolved: facebook/rocksdb#7419 Test Plan: make -j32 Reviewed By: pdillinger Differential Revision: D23883196 Pulled By: zhichao-cao fbshipit-source-id: 94676c26bc56144cc32e3661f84f21eccd790411 Signed-off-by: Changlong Chen <levisonchen@live.cn>

Summary: in PR facebook/rocksdb#7419 , we introduce the new Append and PositionedAppend APIs to WritableFile at File System, which enable RocksDB to pass the data verification information (e.g., checksum of the data) to the lower layer. In this PR, we use the new API in WritableFileWriter, such that the file created via WritableFileWrite can pass the checksum to the storage layer. To control which types file should apply the checksum handoff, we add checksum_handoff_file_types to DBOptions. User can use this option to control which file types (Currently supported file tyes: kLogFile, kTableFile, kDescriptorFile.) should use the new Append and PositionedAppend APIs to handoff the verification information. Pull Request resolved: facebook/rocksdb#7523 Test Plan: add new unit test, pass make check/ make asan_check Reviewed By: pdillinger Differential Revision: D24313271 Pulled By: zhichao-cao fbshipit-source-id: aafd69091ae85c3318e3e17cbb96fe7338da11d0 Signed-off-by: Changlong Chen <levisonchen@live.cn>