fix: let etcd v3 read_watch handle the case where a chunk contains partial event or multiple events
#154
Conversation
…partial event or multiple events
|
|
|
We have a tricky problem, our CI includes a test of ETCD version 3.3.0, but this version of ETCD has a bug in the grpc-gateway dependency it relies on to implement HTTP API, which causes every event json to not have a trailing newline, which is the key I use to split the event. References:
To summarize, ETCD uses grpc-gateway to implement the HTTP API, and grpc-gateway explicitly states that it will use newlines as a separator for stream-type grpc responses, but because of a bug introduced in the historical version, it does not pass the test cases of v3.3.0. |
|
@tokers @starsz @membphis @spacewander Any idea? |
|
we can skip some test case for etcd |
|
We can drop the support of etcd < 3.4 |
Got it, thanks. |
| return nil, err | ||
| body = chunk | ||
| else | ||
| body = body .. chunk |
There was a problem hiding this comment.
Could we use table.concat to avoid creating too much temp str?
lib/resty/etcd/v3.lua
Outdated
| end | ||
| if not utils.is_empty_str(body) then | ||
|
|
||
| if not utils.is_empty_str(chunk) and sub_str(chunk, -1) == "\n" then |
There was a problem hiding this comment.
Use string.byte would be better
| end | ||
| if not utils.is_empty_str(body) then | ||
|
|
||
| if not utils.is_empty_str(chunk) and str_byte(chunk, -1) == str_byte("\n") then |
There was a problem hiding this comment.
So if the response chunk doesn't contain the \n and we'll continue reading the next chunk? But if the connection is aborted / timed out this time, so the last chunk was missed?
There was a problem hiding this comment.
For this case, the caller can rewatch based on the last revision it got, so it can start consuming from the correct revision again without miss any event.
lib/resty/etcd/v3.lua
Outdated
| return nil, err | ||
| body = chunk | ||
| else | ||
| body = tab_concat({body, chunk}) |
There was a problem hiding this comment.
I think we can call tab_contat out of the while.
There was a problem hiding this comment.
Not good, loop may execute more than twice.
There was a problem hiding this comment.
this way is worse than body = body .. chunk
There was a problem hiding this comment.
Not good, loop may execute more than twice.
I mean just tab_concat once to concat all chunk
There was a problem hiding this comment.
Got it, I misunderstand advice from spacewander.
There was a problem hiding this comment.
I thought about it carefully, and since this concat branch will only be executed in rare cases(while one chunk larger than nginx proxy_buffer_size), so I think it would be more appropriate to use a string directly, which avoid creating a table with only one element in most cases.
@membphis @spacewander @starsz Do you agree with this conclusion?
There was a problem hiding this comment.
I think just use .. is enough.
There was a problem hiding this comment.
We can use .. but add a comment to descript why?
Co-authored-by: Alex Zhang <zchao1995@gmail.com>
| if not chunk then | ||
| break | ||
| end | ||
|
|
||
| if not body then |
There was a problem hiding this comment.
Look like this branch is not executed, as body has default value ""?
There was a problem hiding this comment.
yeah, my fault, body should initial with nil.
lib/resty/etcd/v3.lua
Outdated
| if event.kv.value then -- DELETE not have value | ||
| event.kv.value = decode_base64(event.kv.value or "") | ||
| event.kv.value = self.serializer.deserialize(event.kv.value) | ||
| local chunks, error = split(body, [[\n]], "jo") |
There was a problem hiding this comment.
Use err would be better? error is a function in Lua.
There was a problem hiding this comment.
OK, but since err is already defined outside of read_watch, if I use err, our lint will complain about duplicate variable names, so I replace error with split_err.
Try to fix #153, we have verified in our environment that can solve that problem, but the test cases still need time to complete.