Issue98fix#102
Conversation
zkResponse was hiding multi-op error in partial response, added new response with error function. Also, fixed test to expect correct errors instead of api errors Fixes etcd-io#98
The current multiop implementation was not properly encoding a single error in a multiop request. If op 1 succeeds, but op 2 fails, then zookeeper should return two responses, one success and one failure of type -1. Fixes etcd-io#98
|
Changes look OK; have you tried the cross-checked integration tests with |
| bs[i] = z.mkSetDataTxnOp(req) | ||
| mresp.Ops[i].Header.Type = opSetData | ||
| case *CheckVersionRequest: | ||
| mresp.Ops[i].Header.Type = opCheck |
There was a problem hiding this comment.
move this after mkCheckVersionPathTxnOp to be consistent with the other cases?
There was a problem hiding this comment.
Suggested change has been made.
I have not been able to get the xchk,docker tests to run yet, they just seem to hang. I can't get them to run for the master branch either.
Do you have any instructions on setting that up?
moved mkCheckVersionPathTxnOp up 1 line to be consistent with other cases Fixes etcd-io#98
The xchk with docker integration tests are broken. The docker files were attempting to download old versions of Kafka and drill that no longer existed on the servers. In addition the RUOK test was causing all tests to hang in the docker mode by causing a NPE Zookeeper and never completely starting up.
Existing Multi() integration test just had panic(wut). Implemented the integration test and fixed one issue identified by it. The multi-op case was returning a different xid than zookeeper when an empty list was passed in to a multi-op
Kafka test was failing due to wrong ports and non-executable run file
|
I have also fixed the integration cross-check tests and implemented the Multi-op test. There are still some failures in the integration tests unrelated to the Multi-op changes than I will try to address later, but all the unit and integration tests pass now. I think everything is ready to go. |
|
Any chance this can be merged soon? |
|
@xiang90, can you look at merging this? It seems to have been silently dropped by the original maintainer... |
5 participants
This fixes the encoding of a partial response to a multi-op request where one succeeds but another fails. The current implementation returns an error with no response, but Kafka expects Zookeeper to still respond with two results, one with a type of opCheck and another with type opError.
This patch allows it to response to each part of a multi-op request individually, using the first error in the list as the overall result. This fixes the NPE in Kafka described in Issue #98