Roblox / rs-consul

This crate provides access to a set of strongly typed apis to interact with consul (https://www.consul.io/)
MIT License
34 stars 23 forks source link

bug in modify index for read key response with recurse set to true #30

Closed priyankat99 closed 1 month ago

priyankat99 commented 1 year ago

the ReadRequestResponse modify_index is currently defined as:

You can even perform blocking queries against entire subtrees of the KV store: if ?recurse is provided, the returned X-Consul-Index corresponds to the latest ModifyIndex within the prefix, and a blocking query using that ?index will wait until any key within that prefix is updated. _pub modifyindex: i64,

however, i don't think the recurse option is implemented correctly. for example

suppose the original data looks something like this: {modifyIndex: 1, key: "test"} {modifyIndex: 2, key: "test/example"}

when i make a blocking query with ReadKeyRequest recurse set to true on the key "test", I would expect a response like this: {modifyIndex: 2, key: "test"} {modifyIndex: 2, key: "test/example"}

since based off the documentation, I would expect the returned x-consul-index for the key "test" to be the latest modify index within the prefix, so in this case 2, from "test/example"

however, im getting this response back even when recurse is set to true: {modifyIndex: 1, key: "test"} {modifyIndex: 2, key: "test/example"}

this is problematic since i want to easily find the latest modifyIndex so i can make a blocking query on the latest modifyIndex.

not sure if i am making a recursive query wrong or if this is a bug for recursive read requests on the kv store.

kushudai commented 1 year ago

Hi @priyankat99 I think this may be a documentation error/ambiguity. The consul server implementation returns all keys that match the given prefix when using recurse. The blocking query implementation ONLY returns when any of the keys are modified after the given index but they will still return all keys with the provided prefix.

I can see the ambiguity with the statement in hindsight. the returned X-Consul-Index corresponds to the latest ModifyIndex within the prefix, and a blocking query using that ?index will wait until any key within that prefix is updated. is worth changing to specify that all keys are returned if any key with the provided prefix has a modify index larger than the requested one.

I wrote a small/silly test to verify this:

    #[tokio::test(flavor = "multi_thread", worker_threads = 4)]
    async fn create_and_read_key_with_recurse() {
        let consul = get_client();
        let test_id = rand::thread_rng().gen::<u32>();
        let key = format!("test/consul/read{}", test_id);
        let string_value = "This is a test";
        let res = create_or_update_key_value(&consul, &key, string_value).await;
        assert_expected_result_with_index(res, None);

        let key2 = format!("{}/with/recurse", key);
        let string_value = "This is a recurse test";
        let res2 = create_or_update_key_value(&consul, &key2, string_value).await;
        assert_expected_result_with_index(res2, None);

        let string_value = "This is a second test";
        let res = create_or_update_key_value(&consul, &key, string_value).await;
        assert_expected_result_with_index(res, None);
        let res = read_key(&consul, &key).await;
        let res2 = read_key(&consul, &key2).await;
        let initial_modify_index = res.unwrap().first().unwrap().modify_index;
        assert!(initial_modify_index > res2.unwrap().first().unwrap().modify_index);

        let new_key = key.clone();
        let read_key_req = ReadKeyRequest {
            key: &new_key,
            recurse: true,
            index: Some(initial_modify_index as u64),
            ..Default::default()
        };

        println!("Requested modify index: {}", initial_modify_index);
        let read_req = consul.read_key(read_key_req);
        let string_value = "This is a third test";
        let res = create_or_update_key_value(&consul, &key, string_value).await;
        assert_expected_result_with_index(res, None);
        println!("read_key_req: {:?}", read_req.await);
    }

This prints all keys with at least one key having a modify index larger than requested.

Requested modify index: 311
read_key_req: Ok([
    ReadKeyResponse { create_index: 309, modify_index: 312, lock_index: 0, key: "test/consul/read255272499", flags: 0, value: Some("This is a third test"), session: None },
    ReadKeyResponse { create_index: 310, modify_index: 310, lock_index: 0, key: "test/consul/read255272499/with/recurse", flags: 0, value: Some("This is a recurse test"), session: None }
])

For your specific use case, you should be able to iterate over all response keys and pick the largest modify index if I understood you correctly. Please correct me if my understanding is inaccurate!

badalex commented 1 month ago

I hit this recently while using recurse=true and deleting a subkey. The problem is modify_index on the existing keys are smaller than the returned index so the next read_key call returns instantly.

I fixed it by making read_key return _index