Call ValidateOptions from SetOptions #5368

maysamyabandeh · 2019-05-29T18:26:34Z

Currently we validate options in DB::Open. However the validation step is missing when options are dynamically updated in ::SetOptions. The patch fixes that.

maysamyabandeh · 2019-05-29T18:27:45Z

db/db_impl.cc

      env_options_for_compaction_ = env_->OptimizeForCompactionTableWrite(
          env_options_for_compaction_, immutable_db_options_);
      versions_->ChangeEnvOptions(mutable_db_options_);
-      env_options_for_compaction_ = env_->OptimizeForCompactionTableRead(


This line seems to have been a mistake.

Why? It doesn't seem misplaced to me.

This line is introduced in #3004
Previously OptimizeForCompactionTableRead would only be used to set env_options_for_read_

rocksdb/db/compaction_job.cc

Line 328 in e629862

env_->OptimizeForCompactionTableRead(env_options, db_options_)),

which is used for reads. env_options_for_compaction_ which is for writes should only go through OptimizeForCompactionTableWrite.
env_options_for_compaction_

env_options_for_compaction_ doesn't necessarily need to be for writes; at least it doesn't seem to be so from the name. I believe it is used in the lower layers to optimize compaction reads as well.

I checked all the usages. The only place it is used for read is here: https://github.com/facebook/rocksdb/blob/master/db/compaction_job.cc#L646-L652
Which explicitly says that it should not be optimized for reads.

If you see OptimizeForCompactionTableRead, it sets the use_direct_reads option. If use_direct_reads is set, it impacts both user and compaction reads. All the table readers opened for either user or compaction reads should be aware of whether to use O_DIRECT or not and need to open the files appropriately.

If you follow further down the link you provided above, and see in TableCache::GetTableReader, a NewRandomAccessFile is opened with these env options.
https://github.com/facebook/rocksdb/blob/master/db/table_cache.cc#L104

And Posix's NewRandomAccessFile makes ample use of use_direct_reads:
https://github.com/facebook/rocksdb/blob/master/env/env_posix.cc#L202

I don't think so. Can you show the line that env_options_for_compaction_ is used for reading?

Moreover, env_options_for_compaction_ is not affected by OptimizeForCompactionTableRead in ::Open (which is the correct behavior) and suddenly changes when ::SetOptions is called.
If we think use_direct_reads should be used for writes (which I highly doubt), then it should be set in OptimizeForCompactionTableWrite.

Discussed offline. Reverting the change for now to have it fixed in a later PR.

facebook-github-bot

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

facebook-github-bot · 2019-05-29T22:22:40Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-05-29T22:47:49Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-05-29T22:57:00Z

@maysamyabandeh has updated the pull request. Re-import the pull request

sagar0

The change looks reasonable to me.
I'll take a deeper look tomorrow to make sure that all the cases are being handled.

facebook-github-bot · 2019-05-30T14:21:04Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-05-30T14:35:46Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot

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

sagar0

The overall flow looks good to me. Just a few comments.

sagar0 · 2019-05-30T18:08:40Z

db/db_impl.cc

      env_options_for_compaction_ = env_->OptimizeForCompactionTableWrite(
          env_options_for_compaction_, immutable_db_options_);
      versions_->ChangeEnvOptions(mutable_db_options_);
-      env_options_for_compaction_ = env_->OptimizeForCompactionTableRead(


Why? It doesn't seem misplaced to me.

sagar0 · 2019-05-30T21:00:33Z

db/db_impl.cc

+    }
+    DBOptions new_db_options =
+        BuildDBOptions(immutable_db_options_, new_options);
+    if (s.ok()) {


lines 919-920 (maybe lines 916-917 too) should also be inside s.ok(), as there is no reason to call BuildDBOptions if GetMutableDBOptionsFromStrings itself has failed.

It would be less optimized. new_db_options has to be outside s.ok() clause so that it could be reused below. However if we separate the declaration from assignment, the compiler might create two option object: 1st with default values, and 2nd when it is assigned. Assuming at SetOptions mostly pass through, this way is more optimized.

that makes sense. Thanks!

sagar0 · 2019-05-30T21:07:22Z

db/db_impl.cc

@@ -912,6 +913,25 @@ Status DBImpl::SetDBOptions(
    InstrumentedMutexLock l(&mutex_);
    s = GetMutableDBOptionsFromStrings(mutable_db_options_, options_map,
                                       &new_options);
+    if (new_options.bytes_per_sync == 0) {
+      new_options.bytes_per_sync = 1024 * 1024;


Curious, why change it from the place where it was earlier? Why does this need to be done before BuildDBOptions ?

Previously there was only one BuildDBOptions at line 964. Now that we have two, I optimized it for the 2nd to use the result of the 1st. For the however there should not be any change between the two. Updating bytes_per_sync was the only such change, so I moved it before the 1st BuildDBOptions.

Where's the second BuildDBOptions? I see only one on line 920. Maybe I missed the other one.

https://github.com/facebook/rocksdb/pull/5368/files#diff-71a34e2e4e097b44064da7d3a2b68bd8L964
It is removed now.

Oh, I thought you were saying there are two calls to BuildDBOptions in the new code.

facebook-github-bot · 2019-05-31T23:31:24Z

@maysamyabandeh has updated the pull request. Re-import the pull request

remove redudant BuildDBOptions remove mistaken OptimizeForCompactionTableRead on write options ValidateOptions in SetOptions make format return correct status exclude invalids from random options minor Disable setting snappy option in Appveyor exclude unsupported compression types from random gen put back OptimizeForCompactionTableRead

facebook-github-bot · 2019-05-31T23:33:06Z

@maysamyabandeh has updated the pull request. Re-import the pull request

sagar0

Thanks. lgtm.

facebook-github-bot

@maysamyabandeh is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-06-04T03:39:01Z

@maysamyabandeh merged this pull request in ae05a83.

…5438) Summary: This affects our "no compression" automated tests. Since PR #5368, DBTest.DynamicMiscOptions has been failing with: db/db_test.cc:4889: Failure dbfull()->SetOptions({{"compression", "kSnappyCompression"}}) Invalid argument: Compression type Snappy is not linked with the binary. Pull Request resolved: #5438 Differential Revision: D15752100 Pulled By: ltamasi fbshipit-source-id: 3f19eff7cafc03b333965be0203c5853d2a9cb71

Summary: Currently we validate options in DB::Open. However the validation step is missing when options are dynamically updated in ::SetOptions. The patch fixes that. Pull Request resolved: facebook#5368 Differential Revision: D15540101 Pulled By: maysamyabandeh fbshipit-source-id: d27bbffd8f0252d1b50bcf59e0a70a278ed937f4

…acebook#5438) Summary: This affects our "no compression" automated tests. Since PR facebook#5368, DBTest.DynamicMiscOptions has been failing with: db/db_test.cc:4889: Failure dbfull()->SetOptions({{"compression", "kSnappyCompression"}}) Invalid argument: Compression type Snappy is not linked with the binary. Pull Request resolved: facebook#5438 Differential Revision: D15752100 Pulled By: ltamasi fbshipit-source-id: 3f19eff7cafc03b333965be0203c5853d2a9cb71

maysamyabandeh requested review from sagar0, siying and miasantreble May 29, 2019 18:26

maysamyabandeh commented May 29, 2019

View reviewed changes

facebook-github-bot added the CLA Signed label May 29, 2019

facebook-github-bot reviewed May 29, 2019

View reviewed changes

sagar0 reviewed May 30, 2019

View reviewed changes

facebook-github-bot reviewed May 30, 2019

View reviewed changes

sagar0 reviewed May 30, 2019

View reviewed changes

maysamyabandeh force-pushed the validatesetoptins branch from aa22224 to 25f9c07 Compare May 31, 2019 23:31

maysamyabandeh force-pushed the validatesetoptins branch from 25f9c07 to cde47c7 Compare May 31, 2019 23:32

sagar0 approved these changes Jun 3, 2019

View reviewed changes

facebook-github-bot reviewed Jun 3, 2019

View reviewed changes

facebook-github-bot closed this in ae05a83 Jun 4, 2019

facebook-github-bot added the Merged label Jun 4, 2019

ltamasi mentioned this pull request Jun 10, 2019

Fix DBTest.DynamicMiscOptions so it passes even with Snappy disabled #5438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call ValidateOptions from SetOptions #5368

Call ValidateOptions from SetOptions #5368

maysamyabandeh commented May 29, 2019

maysamyabandeh May 29, 2019

sagar0 May 30, 2019 •

edited

Loading

maysamyabandeh May 30, 2019

sagar0 May 30, 2019

maysamyabandeh May 30, 2019

sagar0 May 30, 2019

maysamyabandeh May 30, 2019

maysamyabandeh May 31, 2019

facebook-github-bot left a comment

facebook-github-bot commented May 29, 2019

facebook-github-bot commented May 29, 2019

facebook-github-bot commented May 29, 2019

sagar0 left a comment

facebook-github-bot commented May 30, 2019

facebook-github-bot commented May 30, 2019

facebook-github-bot left a comment

sagar0 left a comment

sagar0 May 30, 2019 •

edited

Loading

sagar0 May 30, 2019

maysamyabandeh May 30, 2019

sagar0 May 30, 2019

sagar0 May 30, 2019

maysamyabandeh May 30, 2019

sagar0 May 30, 2019

maysamyabandeh May 30, 2019

sagar0 May 30, 2019

facebook-github-bot commented May 31, 2019

facebook-github-bot commented May 31, 2019

sagar0 left a comment

facebook-github-bot left a comment

facebook-github-bot commented Jun 4, 2019

Call ValidateOptions from SetOptions #5368

Call ValidateOptions from SetOptions #5368

Conversation

maysamyabandeh commented May 29, 2019

Choose a reason for hiding this comment

sagar0 May 30, 2019 • edited Loading

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

facebook-github-bot commented May 29, 2019

facebook-github-bot commented May 29, 2019

facebook-github-bot commented May 29, 2019

sagar0 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 30, 2019

facebook-github-bot commented May 30, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

sagar0 left a comment

Choose a reason for hiding this comment

sagar0 May 30, 2019 • edited Loading

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 commented May 31, 2019

facebook-github-bot commented May 31, 2019

sagar0 left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 4, 2019

sagar0 May 30, 2019 •

edited

Loading

sagar0 May 30, 2019 •

edited

Loading