Fix loss in segment announcements when segments do not fit in zNode

[anubhgup]

… znodes during compress mode.

[xvrl]

@anubhgup would you mind adding a unit test that exposes this problem? thanks!

[nishantmonu51]

#2268 also seems to address same issue,
@anubhgup @navis, If they are duplicate, can you both sync up and close one of them.

[anubhgup]

@navis @nishantmonu51

Some points in favour of this CL :)

1. I had reported the bug on the Druid devlopment list, and also fixed it first.
   https://groups.google.com/forum/#!searchin/druid-development/from$3Ame/druid-development/OyiL8jC8fw8/1sW5oLCPBQAJ
2. Since I am hitting it in my production setup, I should be able to test the fix also in production.

[drcrallen]

@anubhgup does @navis 's test pass here?

[anubhgup]

@drcrallen @navis I have added Navis's test to the CL. It is passing.

[xvrl]

technically we don't need to remove the node, since it is possible for another segment announcement to fit in there, just not this specific one.

[anubhgup]

Yes, I was debating about this too. I remove it because most announcements will have a similar size, unless there are significant schema changes across batches.

[xvrl]

maybe just add a comment just to explain the reasoning.

[xvrl]

@anubhgup @navis I don't have a strong preference on which PR to merge.

The only difference is that @anubhgup removes a node on the list even if it could potentially be used for a different segment, although that might be unlikely if all announcements have comparable sizes. The only benefit I can see is that it would reduce the number of nodes to scan when deciding which node to add to. It may also increase the number of znodes in theory, but I'm not sure any of those things matter in practice.

I like that @navis added the new vs. existing node in the logs. Since we already merged the test into this PR, we can probably add the log message changes and merge @anubhgup's since he first reported this issue?

[anubhgup]

Added the log message changes.

[nishantmonu51]

👍

[xvrl]

👍 once we add a comment here https://github.com/druid-io/druid/pull/2266/files#r49796880

[xvrl]

@navis any objections to merging this one instead of yours?

[navis]

@xvrl No problem. It's my bad not checked it's already PRed. Thanks to all.