isamu / rocksdb-ruby

A simple RocksDB library for Ruby
MIT License
75 stars 24 forks source link

Don't overwrite readonly if other options are given in hash format. #25

Closed Sander-V-D closed 3 years ago

Sander-V-D commented 3 years ago

Currently, the following code

RocksDB.open_readonly('path', keep_log_file_num: 5)

opens the RocksDB in read-write mode. This is because the passed options overwrite the read-only setting, even if no read-only option is present.

This PR tries to remedy that by using ||= instead of = in the read-only setting assignment.

Another solution to this inconsistency could be to remove the ability to set the read-only setting in the passed options altogether, since it's not possible to do this if you pass the options as a string either.

katafrakt commented 3 years ago

Thank you for finding this issue!

My few thoughts:

  1. IMO this still creates some potential confusion. If readonly is specified in rocksdb_options it won't override previous setting. This might be good or bad, but certainly you cannot tell without looking at the code.
  2. If I understand correctly, rocksdb_options shall reflect this list. Readonly option is not there. So it probably should not be supported via rocksdb_options at all. Perhaps even there should be a list of keys that can be used in rocksdb_options and exception raised if something outside of this list is passed. Note that this would definitely be a breaking change.
Sander-V-D commented 3 years ago
  1. You're right: RocksDB.open_readonly('path', readonly: false) will never behave as expected, because the expectation is not clear.
  2. I also agree here: The clearest solution is to remove setting readonly from the options altogether. rocksdb_options then refers to the right list, and readonly is defined at one spot, not allowing multiple contradictory definitions.

I'll update the PR.

isamu commented 3 years ago

@Sander-V-D Thank you for your good suggestions and patch. @katafrakt Thank you for your good discussion.