dsmiley commented on code in PR #2276: URL: https://github.com/apache/solr/pull/2276#discussion_r1573856139
########## solr/core/src/test-files/solr/collection1/conf/solrconfig-follower-auth.xml: ########## Review Comment: I don't see a reference to this file from the test; am I missing something? If this is actually used, then please consider instead modifying your test to manipulate the temp config files so that we don't have yet another test config file to maintain. For an example of this technique, see org.apache.solr.search.TestCpuAllowedLimit#createConfigSet ########## solr/core/src/java/org/apache/solr/handler/IndexFetcher.java: ########## @@ -1867,7 +1866,6 @@ private int fetchPackets(FastInputStream fis) throws Exception { log.debug("Fetched and wrote {} bytes of file: {}", bytesDownloaded, fileName); // errorCount is always set to zero after a successful packet errorCount = 0; - if (bytesDownloaded >= size) return 0; Review Comment: FWIW it's a bizarre check to me so I'm glad you are removing it. ########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java: ########## @@ -867,6 +869,8 @@ public static class Builder protected Long keyStoreReloadIntervalSecs; + private List<HttpListenerFactory> listenerFactory; Review Comment: There's no setter for this. Which is suspicious... like do we even need this in this PR? If you add a setter for it, you can use the setter in UpdateHandler. ########## solr/core/src/java/org/apache/solr/handler/IndexFetcher.java: ########## @@ -1831,21 +1814,27 @@ private void fetch() throws Exception { private int fetchPackets(FastInputStream fis) throws Exception { byte[] intbytes = new byte[4]; byte[] longbytes = new byte[8]; + boolean isContentReceived = false; Review Comment: We probably don't need this variable anymore; can rely on bytesDownloaded > 0 condition. ########## solr/core/src/java/org/apache/solr/handler/IndexFetcher.java: ########## @@ -1831,21 +1814,27 @@ private void fetch() throws Exception { private int fetchPackets(FastInputStream fis) throws Exception { byte[] intbytes = new byte[4]; byte[] longbytes = new byte[8]; + boolean isContentReceived = false; try { while (true) { + if (fis.peek() == -1) { + return NO_CONTENT; Review Comment: Shouldn't we add this log here as seenbelow: if (!isContentReceived) log.warn("No content received for file: {}", fileName); ########## solr/core/src/java/org/apache/solr/handler/IndexFetcher.java: ########## @@ -1831,21 +1814,27 @@ private void fetch() throws Exception { private int fetchPackets(FastInputStream fis) throws Exception { byte[] intbytes = new byte[4]; byte[] longbytes = new byte[8]; + boolean isContentReceived = false; try { while (true) { + if (fis.peek() == -1) { + return NO_CONTENT; + } if (stop) { stop = false; aborted = true; throw new ReplicationHandlerException("User aborted replication"); } long checkSumServer = -1; + fis.readFully(intbytes); // read the size of the packet int packetSize = readInt(intbytes); if (packetSize <= 0) { - log.warn("No content received for file: {}", fileName); - return NO_CONTENT; + if (!isContentReceived) log.warn("No content received for file: {}", fileName); Review Comment: Since we don't return here, this probably isn't where we should log this message. Nonetheless if we get to this spot, it's now highly unexpected since even if there was no content, the peek==-1 above would happen and we wouldn't get here. So what do we do? Hmm. If we think it's impossible (I think?), add an assert. Could also log a warning. ########## solr/core/src/java/org/apache/solr/handler/IndexFetcher.java: ########## @@ -261,22 +260,23 @@ public String getMessage() { } } - private static HttpClient createHttpClient( - SolrCore core, - String httpBasicAuthUser, - String httpBasicAuthPassword, - boolean useCompression) { + private Http2SolrClient createHttpClient( + SolrCore core, String httpBasicAuthUser, String httpBasicAuthPassword, String leaderBaseUrl) { final ModifiableSolrParams httpClientParams = new ModifiableSolrParams(); Review Comment: Do you see my point that `httpClientParams` is now unused? At a minimum, it needs to be removed now. With your insights on httpBasicAuthUser and httpBasicAuthPassword configuration, a comment explaining *why* we can't simply assume the PKI one would be helpful to whoever reads this in the future. -- 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