search

curiosity-ai / rocksdb-sharp

.net bindings for the rocksdb by facebook
BSD 2-Clause "Simplified" License
155 stars 38 forks source link

Option for InfoLogLevel implemented in wrong class #27

Closed queequac closed 1 year ago

queequac commented 2 years ago

I wanted to change the log level and realized that the wrapper implements SetInfoLogLevel in ColumnFamilyOptions instead of DbOptions.

As a result, one cannot set the log level for the DB. And doing so on the column families is without any effect.

I moved the method accordingly and added also an enum for the available log levels to make the option more readable. I tested it locally and log output follows now the respective option that is set.

queequac commented 2 years ago

@theolivenbaum Argh, this seems to apply for several options. Wanted to configure SetTargetFileSizeBase, which should also belong to the database, and again it is within the ColumnFamilyOptions. :( Would make sense to revisit all options...

theolivenbaum commented 2 years ago

Nice that explains why it never worked for me too! I'll review the PR tomorrow and merge it

queequac commented 2 years ago

Had a deeper look... the problem is that DbOptions inherit from ColumnFamilyOptions... but those defined in the base class for column families return ColumnFamilyOptions... so you cannot use it like fluent API because the return type might not match.

Long story short: My PR handles only the info log level. But we'd have to move all options into the correct bucket. If it cannot be applied to column families, then it should not be part of that class. Not sure how many options are actually shared between column families and db in general. Would propose to keep them separated and not to inherit DbOptions from ColumnFamilyOptions at all.

Have a look at the small PR first... you might merge this one as a first step, and I'd volunteer to refactor options as a whole. But we should open a separate issue for that one then.

queequac commented 1 year ago

@theolivenbaum Found a nice solution for the overal problem mentioned above. Please review and merge this. Thanks. :)

queequac commented 1 year ago

@theolivenbaum Did you have any chance to review this PR? Would really appreciate to have the correct options available in production.

theolivenbaum commented 1 year ago

Hi @queequac - I've merged the PR now, but there's something else broken in the build that I need to fix before we'll have a new version. I'll let you know

theolivenbaum commented 1 year ago

Merged and fixed issue with build, should be available in the next nuget version!