gerlowskija commented on code in PR #3176:
URL: https://github.com/apache/solr/pull/3176#discussion_r1955432972
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java:
##########
@@ -192,7 +192,7 @@ protected SolrClient getClient(Endpoint endpoint) {
*/
@Deprecated
@Override
- public String removeSolrServer(String server) {
+ public synchronized String removeSolrServer(String server) {
Review Comment:
[Q] The javadocs point to `removeSolrEndpoint` as the successor to this
deprecated method: should that be `synchronized` as well?
(It probably doesn't matter terribly for branch_9x since removeSolrEndpoint
currently calls removeSolrServer; mostly mentioning to make sure we remember to
do it on the 'main' version of this PR)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -670,60 +662,64 @@ public RequestWriter getRequestWriter() {
private void checkAZombieServer(ServerWrapper zombieServer) {
try {
+ log.debug("Checking zombie server {}", zombieServer);
QueryRequest queryRequest = new QueryRequest(solrQuery);
final var responseRaw = doRequest(zombieServer.getBaseUrl(),
queryRequest, null);
final var resp = new QueryResponse();
resp.setResponse(responseRaw);
if (resp.getStatus() == 0) {
// server has come back up.
- // make sure to remove from zombies before adding to the alive list to
avoid a race
- // condition
- // where another thread could mark it down, move it back to zombie,
and then we delete
- // from zombie and lose it forever.
- ServerWrapper wrapper =
zombieServers.remove(zombieServer.getBaseUrl());
- if (wrapper != null) {
- wrapper.failedPings = 0;
- if (wrapper.standard) {
- addToAlive(wrapper);
- }
- } else {
- // something else already moved the server from zombie to alive
- }
+ reviveZombieServer(zombieServer);
Review Comment:
[+1] Very nice little cleanup 🎉
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -612,6 +603,7 @@ private void startAliveCheckExecutor() {
if (aliveCheckExecutor == null) {
synchronized (this) {
if (aliveCheckExecutor == null) {
+ log.debug("Starting aliveCheckExecutor");
Review Comment:
[0] Not sure if you intend to keep these debug lines, but if so, might be
worth adding something to the message to indicate which client it belongs to,
since apps might have multiple.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
