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
