You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/08/30 14:52:57 UTC
[GitHub] [druid] capistrant opened a new issue, #12999: I don't think that ChangeRequestHistory should force a Counter reset if the Counter matches the latest Counter when waiting futures are resolved
capistrant opened a new issue, #12999:
URL: https://github.com/apache/druid/issues/12999
### Affected Version
0.22.2
### Description
```
2022-08-30T14:10:38,628 INFO [HttpServerInventoryView-6] org.apache.druid.server.coordination.ChangeRequestHttpSyncer - [https://XXXXXX.XXXXXX.com:8283/_1661264760506] requested resetCounter for reason [counter[Counter{counter=94953, hash=1661868638606}] >= last counter[Counter{counter=94953, hash=1661868638606}]].
```
You can see here that the client request for changes on this historical provides a Counter that matches the last counter on the server. This causes the client to send a fresh request to the server with a null counter in order to get the full set of segments. I am struggling to understand why we would want to reset a server that is up to date.
It looks like [this](https://github.com/apache/druid/blob/0.22.2/server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java#L146) is what is causing the Counter to be reset.
It seems that the above code path is run either when the Counter **does not match** when the client request is made OR when the waiting futures (requests waiting for a new change) are resolved [here](https://github.com/apache/druid/blob/0.22.2/server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java#L197). It is this latter case that I don't understand the reset Counter return to client? Why not send some empty response and the client comes back with the same counter looking for a new change? Resetting it feels like overkill and added work for no gain. Perhaps I'm missing something here, but as of now it feels like incorrect, albeit not destructive, behavior.
spit balling on change to [the code that controls resetting the Counter](https://github.com/apache/druid/blob/0.22.2/server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java#L146)
```JAVA
if (counter.counter == lastCounter.counter) {
if (counter.matches(lastCounter)) {
// return some empty response that does not cause client to reset counter
} else {
// this is bad, fail with a reset due to an incorrect hash - unlikely this would happen here, but it adds safety
}
} else if (counter.counter > lastCounter.counter) { // This is modification of original conditional to just GT
return ChangeRequestsSnapshot.fail(
StringUtils.format(
"counter[%s] > last counter[%s]",
counter,
lastCounter
)
);
} else if ...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #12999: I don't think that ChangeRequestHistory should force a Counter reset if the Counter matches the latest Counter when waiting futures are resolved
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #12999:
URL: https://github.com/apache/druid/issues/12999#issuecomment-1232163768
One way I could see this happening is if [this](https://github.com/apache/druid/blob/0.22.2/server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java#L80) was called with an empty list. The only place I can tell that this would even be possible is [here](https://github.com/apache/druid/blob/0.22.2/server/src/main/java/org/apache/druid/server/coordination/BatchDataSegmentAnnouncer.java#L236)... but that should only be running during bootstrap and I have confirmed in my logs that I see the is finished log for the background announcer that runs during startup.
Either way, I wonder if it makes sense to put a conditional in place to only clear the futures if that first link gets called with a list of non-zero length to prevent resolving the futures when the counter hasn't incremented
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] AmatyaAvadhanula closed issue #12999: I don't think that ChangeRequestHistory should force a Counter reset if the Counter matches the latest Counter when waiting futures are resolved
Posted by "AmatyaAvadhanula (via GitHub)" <gi...@apache.org>.
AmatyaAvadhanula closed issue #12999: I don't think that ChangeRequestHistory should force a Counter reset if the Counter matches the latest Counter when waiting futures are resolved
URL: https://github.com/apache/druid/issues/12999
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #12999: I don't think that ChangeRequestHistory should force a Counter reset if the Counter matches the latest Counter when waiting futures are resolved
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #12999:
URL: https://github.com/apache/druid/issues/12999#issuecomment-1232114601
Upon reviewing the code further, I'm struggling to identify how we even get into a case where we need to reset because the counters match for both counter number and hash. @clintropolis You mention investigating those warning logs in your patch, #12209, do you remember if those full syncs were getting caused by this same counter reset due to the counter from the client being `>=` to the latest counter when the change future is being processed on the data server?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #12999: I don't think that ChangeRequestHistory should force a Counter reset if the Counter matches the latest Counter when waiting futures are resolved
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #12999:
URL: https://github.com/apache/druid/issues/12999#issuecomment-1240859129
I think #13010 is a safe way to solve this. There is some unnecessary no-op network traffic, but it should put a stop to wasteful counter resets
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on issue #12999: I don't think that ChangeRequestHistory should force a Counter reset if the Counter matches the latest Counter when waiting futures are resolved
Posted by GitBox <gi...@apache.org>.
clintropolis commented on issue #12999:
URL: https://github.com/apache/druid/issues/12999#issuecomment-1232231937
>@clintropolis You mention investigating those warning logs for duplicate segments in your patch, https://github.com/apache/druid/pull/12209, do you remember if those full syncs that were triggering the logs were getting caused by this same counter reset due to the counter from the client being >= to the latest counter when the change future is being processed on the data server?
😅 I unfortunately don't remember so well, but maybe it sounds familiar? I think what you are suggesting makes sense, though obligatory disclaimer that I'm not super familiar with this part of the code myself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org