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

Add API call for retrieving db index #285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

unitink72
Copy link
Contributor

Its hard to use the ETCD watch API without knowing the current DB index, so I added it. It works when I test it. Hopefully the method of doing a GET on host/v2/keys just to get the HTTP header is acceptable.

@Oipo
Copy link
Contributor

Oipo commented Sep 24, 2020

So X-Etcd-Index is tied to an etcd server (so in a cluster, you can have multiple I think, not sure).
But the index used in the api is the modifiedIndex stored with a key.

If we take a look at the etcd API description:

node.createdIndex: an index is a unique, monotonically-incrementing integer created for each change to etcd. This specific index reflects the point in the etcd state member at which a given key was created. You may notice that in this example the index is 2 even though it is the first request you sent to the server. This is because there are internal commands that also change the state behind the scenes, like adding and syncing servers.

node.modifiedIndex: like node.createdIndex, this attribute is also an etcd index. Actions that cause the value to change include set, delete, update, create, compareAndSwap and compareAndDelete. Since the get and watch commands do not change state in the store, they do not change the value of node.modifiedIndex.

It's probably best to do an etcdlib_get() before an etcdlib_watch(). If the return value is ETCDLIB_RC_ERROR, you can assume it is 0.

Verifying this locally:

oipo@oipo-X570-AORUS-ELITE:~$ curl http://127.0.0.1:2379/v2/keys/message3
{"errorCode":100,"message":"Key not found","cause":"/message3","index":5266}
oipo@oipo-X570-AORUS-ELITE:~$ curl http://127.0.0.1:2379/v2/keys
{"action":"get","node":{"dir":true,"nodes":[{"key":"/hier","dir":true,"modifiedIndex":13,"createdIndex":13}]}}

@unitink72
Copy link
Contributor Author

I had two factors that led me to wanting this available:

  1. When I wrote my db interface code, the first thing I did was setup a directory/key structure then set a watch on the etcd directory containing all those keys. Using the index from the last get always resulted in the watch api being triggered on changes that were made in the past. There was an ugly work around that by writing a key that was outside that dir structure then reading it again, but that was a hack.

  2. In the link you gave (etcd v2 api), there's a section down further titled "Watch From Cleared Event Index". It describes a case that could happen in my system - the modified index of the key being over 1000 revisions old. This would break my watch api calls.

It may be possible to have multiple X-Etcd-Index values among a cluster, but I'm assuming that they try to sycn them up asap. That issue doesn't bother me for my system.

If the code doesnt fit for your project, thats OK. Just thought I'd offer it up as its definitely useful for me.

Copy link
Contributor

@Oipo Oipo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the back and forth, making me read the documentation more carefully. I agree that there is a good reason to request the current X-Etcd-Index value before making a watch request. Having a specific function to return that would be valuable. I guess we also need to have etcdlib_watch() return the X-Etcd-Index value if error code 401 is detected in the json response.

If you don't feel like making that last change, let me know. I'll do it later.

* @param const etcdlib_t* etcdlib. The ETCD-LIB instance
* @param int* modifiedIndex. The X-Etcd-Index value as retrieved from the etcd server.
*/
int etcdlib_get_db_index(etcdlib_t *etcdlib, int* modifiedIndex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename modifiedIndex to currentEtcdIndex

@@ -257,6 +257,40 @@ static long long etcd_get_current_index(const char* headerData) {
}


int etcdlib_get_db_index(etcdlib_t *etcdlib, int* modifiedIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename modifiedIndex to currentEtcdIndex

fprintf(stderr, "Error getting etcd value, curl error: '%s'\n", curl_easy_strerror(res));
}

if (reply.memory) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it happens multiple times in the current code, would you mind removing the if statement here? free() explicitly handles NULL values properly by ignoring them.

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.

2 participants