HH should not process dropped nodes #4377
Conversation
|
As for testing, I thought the simplest thing to do was to add the new code to the existing test. Yes, it means a somewhat longer test case, but it's very straightforward. This is the limit to which I would push it though. |
|
Patch updated to remove the stubbed-out "purge" call. All data deletion is left to simple expiration so that data is never dropped directly due to operations such as DROP SERVER. |
|
I see that with this change, the system will continually be adding a new segment for all queues it purges. That is not what I wanted. Let me fix this up. |
otoolep
force-pushed
the
dropped_server_hh
branch
3 times, most recently
from
adc021d
to
d47521f
Compare
| @@ -273,6 +281,20 @@ func (p *Processor) updateShardStats(shardID uint64, stat string, inc int64) { | |||
| m.Add(stat, inc) | |||
| } | |||
|
|
|||
| func (p *Processor) activeNodes() ([]uint64, error) { | |||
There was a problem hiding this comment.
This could also be activeQueues, I could go either way.
otoolep
force-pushed
the
dropped_server_hh
branch
from
d47521f
to
b4d7243
Compare
otoolep
force-pushed
the
dropped_server_hh
branch
from
b4d7243
to
c53d553
Compare
|
OK, this is ready to go @jwilder and @corylanou Lots of way to solve this, I think this should do it nicely. |
| if err := queue.Close(); err != nil { | ||
| return err | ||
| } | ||
| if err := os.RemoveAll(queue.dir); err != nil { |
There was a problem hiding this comment.
Seems like this be a func on queue. queue.Remove()
There was a problem hiding this comment.
Yeah, I thought about that, but wasn't sure. Should Remove() work on an open-queue? Should the queue be closed first? What are the semantic of this command?
All these reasons made me fall back on having this operation occur outside of the queue. (I started with having it as a method on the queue, as you suggest). I don't feel super strongly about this, and wasn't too happy with the code acting on path, technically an unexported variable (but fine obviously within the package).
I'm happy with whatever is clearest to the next guy.
There was a problem hiding this comment.
I'd say return an error if you call Remove and the queue is not already closed.
There was a problem hiding this comment.
OK, sounds good, I'll do that.
|
A couple of small things but otherwise LGTM. 👍 |
otoolep
force-pushed
the
dropped_server_hh
branch
from
c53d553
to
bfbf484
Compare
|
@jwilder -- there still seems to me to be an issue in |
otoolep
force-pushed
the
dropped_server_hh
branch
from
bfbf484
to
71bf3c4
Compare
Deletion only takes place if all data in the queue is older than the configured time.
otoolep
force-pushed
the
dropped_server_hh
branch
from
71bf3c4
to
b009f25
Compare
|
Feedback from @jwilder integrated, merging now. |
HH should not process dropped nodes
|
@otoolep Looks like an existing bug. Should be straightforward to fix. |
|
OK, I'll take care of it @jwilder. |
No description provided.