Copy iterator interrupt #681
Conversation
abudnik
commented
Dec 22, 2015
abudnik
commented
- copy-iterator: stop iteration only if connection failed or no space left on remote backend
- server-send: check that source and destination backends are not the same
- server-send: added 80 reserved bytes to dnet_server_send_request struct
| @@ -783,6 +785,25 @@ int dnet_server_send_write(struct dnet_server_send_ctl *send, | |||
| (unsigned long long)re->iterated_keys, (unsigned long long)re->total_keys, | |||
| atomic_read(&send->bytes_pending), send->bytes_pending_max); | |||
|
|
|||
| // check that source and destination backends are not the same | |||
There was a problem hiding this comment.
Shouldn't it be on the client side?
Do not even try to start iterator if its source/destination backends are the same?
There was a problem hiding this comment.
No, this happens when client state gone during iteration. It starts to write to itself.
There was a problem hiding this comment.
Ok, I see. Please add extended comments about this case
| ctl.id.group_id = group_id; | ||
| const struct dnet_net_state *st = dnet_state_get_first_with_backend(n, &ctl.id, &backend_id); | ||
| if (st == n->st && backend_id == send->backend_id) { | ||
| dnet_log(n, DNET_LOG_ERROR, "%s: Interrupting iterator: source and destination backends are the same: " |
There was a problem hiding this comment.
Does it really interrupt iterator in this case or it just skips the key?
There was a problem hiding this comment.
It interrupts iterrator. I've manually tested this code with iptables before sending PR :)
There was a problem hiding this comment.
How can it be tested via iptables? It's about writing keys when destination and source are the same backend.
There was a problem hiding this comment.
It interrupts iterrator.
There was a problem hiding this comment.
Iterator's callback returns error code from dnet_server_send_write(), so iteration stops on this error.
There was a problem hiding this comment.
I've tested manually with iptables closing remote connection, so this case was reproduced and iteration stopped.
There was a problem hiding this comment.
It shouldn't interrupt iteration here and there are few reasons for that:
- it should avoid writing keys to the same backend, but it should write all keys to other groups
- if there is only one destination group, it should skip keys that should be written to the same backend, but it should write all keys to other backends
There was a problem hiding this comment.
I agree and will fix it soon.
abudnik
force-pushed
the
copy_iter_interrupt
branch
from
f5af2a3
to
ae161ba
Compare
| @@ -671,6 +672,28 @@ static int dnet_server_send_sync(struct dnet_server_send_ctl *ctl) | |||
| return dnet_server_send_put(ctl); | |||
| } | |||
|
|
|||
| static int dnet_filter_invalid_groups(int *groups, const struct dnet_server_send_ctl *send, struct dnet_node *n, struct dnet_id *id) | |||
There was a problem hiding this comment.
This requires more comments about the whole logic behind this function.
Why is it needed, what does it do, how does it modify group array.
One should understand this function without looking into the code, but from description.
Looking into the code is only needed when you want to fix a bug.
It also leaks state.
abudnik
force-pushed
the
copy_iter_interrupt
branch
2 times, most recently
from
4782b72
to
2b37492
Compare
…eft on remote backend
…backends are the same
abudnik
force-pushed
the
copy_iter_interrupt
branch
from
2b37492
to
62b1d29
Compare
abudnik
force-pushed
the
copy_iter_interrupt
branch
from
c78ce00
to
c21d415
Compare
