Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

!fix: Ensure default column family uses specified open options #882

Merged

Conversation

0xdeafbeef
Copy link
Contributor

Previously, the default column family was initialized with default settings if not explicitly included in the column family descriptors. This commit ensures that the default column family inherits the open options

@zaidoon1
Copy link
Contributor

zaidoon1 commented May 5, 2024

I'm hesitant to make this change. The existing api clearly states that the cfs provided will get the provided options applied so if the user doesn't provide the default cf then the default cf will get the default settings which makes sense to me. The issue here is that this change is a breaking change that will change behaviour for users that have not set anything for the default cf which may break people... It might be better to document the functions to state that the default cf will get the default options if it's not included/not specifically configured.

@stanislav-tkach
Copy link
Member

I also thought that it is breaking change and with current rust-rocksdb versioning it is impossible to indicate that to the users. I tend to agree with @zaidoon1.

@0xdeafbeef
Copy link
Contributor Author

@stanislav-tkach @zaidoon1

    #[test]
    fn test_default_cf_compaction() {
         use rocksdb::{CompactionDecision, Options, DB};
         
        let mut cf_opts = Options::default();
        cf_opts.set_compaction_filter("test", |_, _, _| {
            println!("compaction_filter in cf");
            CompactionDecision::Keep
        });

        let descriptor = rocksdb::ColumnFamilyDescriptor::new("test", cf_opts);

        let mut opts = Options::default();
        opts.create_if_missing(true);
        opts.create_missing_column_families(true);
        opts.set_compaction_filter("test", |_, _, _| {
            println!("compaction_filter in DEFAULT");
            CompactionDecision::Keep
        });
        let db = DB::open_cf_descriptors(&opts, "test", vec![descriptor]).unwrap();
        let cf = db.cf_handle("test").unwrap();
        db.put_cf(cf, b"1", b"").unwrap();
        db.compact_range_cf(cf, None::<&[u8]>, None::<&[u8]>);

        db.put(b"key", "").unwrap();
        db.compact_range(None::<&[u8]>, None::<&[u8]>);
        drop(db);

        DB::destroy(&opts, "test").unwrap();
    }

In this case, compaction_filter in cf will be printed and compaction_filter in DEFAULT won't.

`open_cf_descriptors` docs says:

Opens a database with the given database options and column family descriptors.

Thus, when you call put on the database, you expect that the options set for the database will be applied.

So if a change breaks backward compatibility, I think we should at least update the docs to make this behavior more clear.

@zaidoon1
Copy link
Contributor

zaidoon1 commented May 7, 2024

I think we should at least update the docs to make this behavior more clear.

I agree, if you can update the pr, that would be great

@0xdeafbeef 0xdeafbeef force-pushed the 0xdeafbeef/push-wmllnkulvuyp branch from 8890da6 to 9f43f16 Compare May 8, 2024 13:45
@0xdeafbeef
Copy link
Contributor Author

I think we should at least update the docs to make this behavior more clear.

I agree, if you can update the pr, that would be great

done

@zaidoon1 zaidoon1 enabled auto-merge (rebase) May 8, 2024 15:21
@zaidoon1 zaidoon1 disabled auto-merge May 8, 2024 15:21
@zaidoon1 zaidoon1 merged commit daaaf85 into rust-rocksdb:master May 8, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants