Add set_consistent_hashed_upstream #29

charliechang · 2016-03-03T03:02:31Z

Hi
I added the support of consistent hash by reuse the directive set_hashed_upstream. I am not sure if there is any alternative for this. (Maybe can be done by Lua?)

agentzh · 2016-03-03T22:18:24Z

@charliechang Thanks for sharing.

@doujiang24 Will you have a quick look at this PR please? Thanks!

doujiang24 · 2016-03-09T10:12:04Z

src/ngx_http_set_hashed_upstream.c

+        for (i = 0; i < hashConf->nodeLen; i++) {
+            hashConf->hashNodes[i].name = ul->elts[i];
+            hashConf->hashNodes[i].hash = ngx_hash_key_lc(hashConf->hashNodes[i].name->data,
+                                                          hashConf->hashNodes[i].name->len);


One real node should have a number of virtual nodes, usually the number is 160, like nginx chash does: https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_upstream_hash_module.c#L299

Otherwise, the consistent hash will have uneven distribution.

doujiang24 · 2016-03-09T10:20:27Z

@charliechang Thanks for sharing. Yes, I think it's better to be done in Lua by lua-resty-chash.
https://github.com/agentzh/lua-resty-chash

The way does in this PR can be work too, but it can be bad performance. May be we can give up this PR? Or may be my wrong? Thoughts?

doujiang24 · 2016-03-10T02:00:56Z

@charliechang Sorry, I mean bad performance is not so good, but in some simple case it's good enough.

charliechang · 2016-03-11T05:10:42Z

@doujiang24
Thanks for your suggestion. You are right. This method will result in uneven distribution. I will fix it in my side, Regarding the performance, I found there is a const-time implementation of consistent hash. Maybe it can help the problem?
https://github.com/replay/ngx_http_consistent_hash/blob/master/ngx_http_upstream_consistent_hash_module.c

charliechang · 2016-03-11T05:15:45Z

@doujiang24
Anyway, you can give up this PR or we can keep discussion. It is your call. Thanks for review.

doujiang24 · 2016-03-11T06:37:35Z

@charliechang Sorry, I make a stupid mistake here, the performance should not be a problem, please forgive me. Let's continue discuss.
I also found some code style things, Better use the ngx-releng tool from the nginx-devel-utils repos to check your code.

doujiang24 · 2016-03-11T06:39:12Z

src/ngx_http_set_hashed_upstream.c

    switch (type) {
-    case ngx_http_set_misc_distribution_modula:
-        return (uint32_t) hash % (uint32_t) ul->nelts;
+        case ngx_http_set_misc_distribution_modula:


case should left align with switch

charliechang · 2016-03-14T02:34:35Z

@doujiang24
I added the support of virtual nodes and fixed the problems you mentioned above. Please let me know if there is anything I can make it better!

doujiang24 · 2016-03-14T08:48:50Z

src/ngx_http_set_hashed_upstream.c

@@ -1,27 +1,71 @@
 #ifndef DDEBUG
 #define DDEBUG 0
 #endif
+


unnecessary blank line here.

doujiang24 · 2016-03-14T09:27:49Z

@charliechang
Nice, it looks good to me now, but some code style things, hope you won't be impatient about this.
You'd better use ngx-releng to check the basic code style, this is the recommended way.
Will you add some test case for this? like t/hashed-upstream.t in this repos.
Some more you may need :)

charliechang · 2016-03-15T00:17:13Z

@doujiang24
No problem, I will check the coding style and add test cases.

agentzh · 2016-03-22T19:30:33Z

src/ngx_http_set_hashed_upstream.c

+    ngx_http_set_hashed_upstream_consistent_hash_node *nodes, uint32_t node_len,
+    uint32_t target)
+{
+    uint32_t m = 0, n = node_len, l;


Style: nginx coding style requires each C function local variable to be defined in separate lines.

agentzh · 2016-03-22T19:33:59Z

@charliechang There are so many coding style issues. Please check the existing code for the right style. Thank you for your contribution!

…ent_hashed_upstream

charliechang · 2016-03-24T02:47:31Z

src/ngx_http_set_hashed_upstream.c

+
+        if (hash_conf->hash_nodes == NULL) {
+            return NGX_CONF_ERROR;
+        }


@agentzh Could you point out where are the indent problems around here? Thanks

@charliechang Sorry, my mistake. I thought the long line breaks were indentation problems.

charliechang · 2016-03-28T13:08:15Z

@agentzh I fixed the coding style issues accordingly. Please check it again. Thanks!

charliechang · 2016-05-03T02:18:52Z

@agentzh
Is this PR still valid? since it is been a while...

jbergstroem · 2017-06-27T16:14:10Z

I this this would be a good addition. Any comments on why it stalled? Better implemented elsewhere?

agentzh · 2017-06-27T22:28:34Z

@jbergstroem Yeah, now that we have balancer_by_lua* and such hashing is already implemented in lua-resty-balancer:

https://github.com/agentzh/lua-resty-balancer/

It's way more flexible and powerful.

jbergstroem · 2017-06-28T04:12:49Z

@agentzh said:
@jbergstroem Yeah, now that we have balancer_by_lua* and such hashing is already implemented in lua-resty-balancer

Of course. Thanks for the pointer. So much to learn! [suggesting we close this then]

Add set_consistent_hashed_upstream

006a760

doujiang24 reviewed Mar 9, 2016
View reviewed changes

doujiang24 reviewed Mar 11, 2016
View reviewed changes

Chang, Charlie added 4 commits March 11, 2016 18:10

Add support of virtual nodes

721e4ae

Use hash ngx_hash_key_loc

821bd00

Fix naming

f55ff22

Adjust line cut from 120 to 80

f23971f

Chang, Charlie added 2 commits March 14, 2016 11:40

Fix naming and line length in ngx_http_set_hashed_upstream.h

0b1ec9a

Fix naming and line length in ngx_http_set_hashed_upstream.h

b12f570

doujiang24 reviewed Mar 14, 2016
View reviewed changes

src/ngx_http_set_hashed_upstream.c

@@ -1,27 +1,71 @@

#ifndef DDEBUG

#define DDEBUG 0

#endif

Copy link

Member

doujiang24 Mar 14, 2016

Choose a reason for hiding this comment

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

unnecessary blank line here.

Manually reformat the coding style

a3cc6db

Chang, Charlie added 5 commits March 21, 2016 12:30

Update test case

2d92411

reverse accidental modified lines

a8aa84d

Update description

d10b87a

reverse accidental modified lines

6f1ad45

Update description

2d6dbb8

agentzh reviewed Mar 22, 2016
View reviewed changes

Chang, Charlie added 4 commits March 24, 2016 11:02

Fix coding style

fd64665

Merge branch 'set_consistent_hashed_upstream_vnodes' into set_consist…

f2e8f7c

…ent_hashed_upstream

Fix coding style

bab0307

Fix coding style

e712d38

charliechang reviewed Mar 24, 2016
View reviewed changes

Fix coding style

da8dddd

agentzh closed this Jun 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add set_consistent_hashed_upstream #29

Add set_consistent_hashed_upstream #29

charliechang commented Mar 3, 2016

agentzh commented Mar 3, 2016

doujiang24 Mar 9, 2016

doujiang24 commented Mar 9, 2016

doujiang24 commented Mar 10, 2016

charliechang commented Mar 11, 2016

charliechang commented Mar 11, 2016

doujiang24 commented Mar 11, 2016

doujiang24 Mar 11, 2016

charliechang commented Mar 14, 2016

doujiang24 Mar 14, 2016

doujiang24 commented Mar 14, 2016

charliechang commented Mar 15, 2016

agentzh Mar 22, 2016

agentzh commented Mar 22, 2016

charliechang Mar 24, 2016

agentzh Mar 24, 2016

charliechang commented Mar 28, 2016

charliechang commented May 3, 2016

jbergstroem commented Jun 27, 2017

agentzh commented Jun 27, 2017

jbergstroem commented Jun 28, 2017

Add set_consistent_hashed_upstream #29

Add set_consistent_hashed_upstream #29

Conversation

charliechang commented Mar 3, 2016

agentzh commented Mar 3, 2016

doujiang24 Mar 9, 2016

Choose a reason for hiding this comment

doujiang24 commented Mar 9, 2016

doujiang24 commented Mar 10, 2016

charliechang commented Mar 11, 2016

charliechang commented Mar 11, 2016

doujiang24 commented Mar 11, 2016

doujiang24 Mar 11, 2016

Choose a reason for hiding this comment

charliechang commented Mar 14, 2016

doujiang24 Mar 14, 2016

Choose a reason for hiding this comment

doujiang24 commented Mar 14, 2016

charliechang commented Mar 15, 2016

agentzh Mar 22, 2016

Choose a reason for hiding this comment

agentzh commented Mar 22, 2016

charliechang Mar 24, 2016

Choose a reason for hiding this comment

agentzh Mar 24, 2016

Choose a reason for hiding this comment

charliechang commented Mar 28, 2016

charliechang commented May 3, 2016

jbergstroem commented Jun 27, 2017

agentzh commented Jun 27, 2017

jbergstroem commented Jun 28, 2017