dsmiley commented on code in PR #115:
URL: https://github.com/apache/solr-sandbox/pull/115#discussion_r1946869972
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java:
##########
@@ -527,31 +512,33 @@ private void commitEncryptionStart(String keyId,
req.getCore().getUpdateHandler().commit(commitCmd);
}
- private void encryptAsync(SolrQueryRequest req, long startTimeNs) {
+ private void encryptAsync(SolrQueryRequest req) {
log.debug("Submitting async encryption");
executor.submit(() -> {
try {
EncryptionUpdateLog updateLog = getEncryptionUpdateLog(req.getCore());
if (updateLog != null) {
log.debug("Encrypting update log");
+ long startTimeNs = getTimeSource().getTimeNs();
boolean logEncryptionComplete = updateLog.encryptLogs();
- log.info("{} encrypted the update log in {}",
- logEncryptionComplete ? "Successfully" : "Partially",
elapsedTime(startTimeNs));
+ log.info("{} encrypted the update log in {} ms",
+ logEncryptionComplete ? "Successfully" : "Partially",
elapsedTimeMs(startTimeNs));
// If the logs encryption is not complete, it means some logs are
currently in use.
// The encryption will be automatically be retried after the next
commit which should
// release the old transaction log and make it ready for encryption.
}
log.debug("Triggering index encryption");
+ long startTimeNs = getTimeSource().getTimeNs();
CommitUpdateCommand commitCmd = new CommitUpdateCommand(req, true);
// Trigger EncryptionMergePolicy.findForcedMerges() to re-encrypt
// each segment which is not encrypted with the latest active key id.
commitCmd.maxOptimizeSegments = Integer.MAX_VALUE;
req.getCore().getUpdateHandler().commit(commitCmd);
- log.info("Successfully triggered index encryption with commit in {}",
elapsedTime(startTimeNs));
+ log.info("Successfully triggered index encryption with commit in {}
ms", elapsedTimeMs(startTimeNs));
Review Comment:
Looks confusing / error-prone. Seeing elapsedTimeMs take a parameter with
"Ns" suffix smells of a bug. I wonder if we should embrace Instant where we
can and thus avoid time units in many places?
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java:
##########
@@ -412,58 +409,46 @@ private void distributeRequest(SolrQueryRequest req,
SolrQueryResponse rsp, Stri
rsp.addToLog(ENCRYPTION_STATE, collectionState.value);
}
if (log.isInfoEnabled()) {
- long timeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startTimeNs);
- log.info("Responding encryption distributed state={} success={} for
keyId={} collection={} timeMs={}",
- (collectionState == null ? null : collectionState.value), success,
keyId, collectionName, timeMs);
+ log.info("Responding encryption distributed state={} success={} for
keyId={} timeMs={}",
+ (collectionState == null ? null : collectionState.value), success,
keyId, elapsedTimeMs(startTimeNs));
}
}
}
- private SolrClientHolder getHttpSolrClient(SolrQueryRequest req) {
- CoreContainer coreContainer = req.getCore().getCoreContainer();
- CloudSolrClient cloudSolrClient =
coreContainer.getSolrClientCache().getCloudSolrClient(coreContainer.getZkController().getZkClient().getZkServerAddress());
- if (cloudSolrClient instanceof CloudHttp2SolrClient) {
- return new SolrClientHolder(((CloudHttp2SolrClient)
cloudSolrClient).getHttpClient(), false);
- }
- return new SolrClientHolder(new Http2SolrClient.Builder().build(), true);
- }
-
protected ModifiableSolrParams
createDistributedRequestParams(SolrQueryRequest req, SolrQueryResponse rsp,
String keyId) {
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.set(PARAM_KEY_ID, keyId == null ? NO_KEY_ID : keyId);
- return params;
- }
-
- boolean isTimeout(long maxTimeNs) {
- return System.nanoTime() > maxTimeNs;
+ return new ModifiableSolrParams().set(PARAM_KEY_ID, keyId == null ?
NO_KEY_ID : keyId);
}
- private State sendEncryptionRequestWithRetry(Replica replica,
ModifiableSolrParams params, Http2SolrClient httpSolrClient, String keyId,
String collection) {
+ private State sendEncryptionRequestWithRetry(
+ Replica replica,
+ String requestPath,
+ ModifiableSolrParams params,
+ Http2SolrClient httpSolrClient,
+ String keyId) {
for (int numAttempts = 0; numAttempts < DISTRIBUTION_MAX_ATTEMPTS;
numAttempts++) {
try {
- NamedList<Object> response = sendEncryptionRequest(replica, params,
httpSolrClient);
- Object responseStatus = response.get(STATUS);
- Object responseState = response.get(ENCRYPTION_STATE);
- log.info("Encryption state {} status {} for replica {} keyId {}
collection={}", responseStatus, responseState, replica.getName(), keyId,
collection);
+ SimpleSolrResponse response = sendEncryptionRequest(replica,
requestPath, params, httpSolrClient);
+ Object responseStatus = response.getResponse().get(STATUS);
+ Object responseState = response.getResponse().get(ENCRYPTION_STATE);
+ log.info("Encryption state {} status {} for replica {} keyId {} in {}
ms", responseStatus, responseState, replica.getName(), keyId,
response.getElapsedTime());
if (responseState != null) {
return State.fromValue(responseState.toString());
}
} catch (SolrServerException | IOException e) {
log.error("Error occurred while sending encryption request", e);
}
}
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Failed
encryption request to replica " + replica.getName() + " for keyId " + keyId + "
collection " + collection);
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Failed
encryption request to replica " + replica.getName() + " for keyId " + keyId);
}
- private NamedList<Object> sendEncryptionRequest(Replica replica,
ModifiableSolrParams params, Http2SolrClient httpSolrClient)
+ private SimpleSolrResponse sendEncryptionRequest(
+ Replica replica,
+ String requestPath,
+ ModifiableSolrParams params,
+ Http2SolrClient httpSolrClient)
throws SolrServerException, IOException {
- GenericSolrRequest request = new
GenericSolrRequest(SolrRequest.METHOD.POST, getPath(), params);
- request.setBasePath(replica.getCoreUrl());
- return httpSolrClient.request(request);
- }
-
- protected String getPath() {
- return "/admin/encrypt";
+ GenericSolrRequest distributedRequest = new
GenericSolrRequest(SolrRequest.METHOD.POST, requestPath, params);
+ return httpSolrClient.requestWithBaseUrl(replica.getCoreUrl(),
replica.getCollection(), distributedRequest);
Review Comment:
if you use getCoreUrl, it's complete (solr (aka "base url") + core).
Passing collection here would be a problem; concatenating that after the core
url which doesn't make sense. A test had better fail here.
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java:
##########
@@ -334,75 +334,72 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
}
} finally {
- if (success) {
- rsp.add(STATUS, STATUS_SUCCESS);
- } else {
- rsp.add(STATUS, STATUS_FAILURE);
- }
+ String statusValue = success ? STATUS_SUCCESS : STATUS_FAILURE;
+ rsp.add(STATUS, statusValue);
+ rsp.addToLog(STATUS, statusValue);
rsp.add(ENCRYPTION_STATE, state.value);
- long timeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
startTimeNs);
+ rsp.addToLog(ENCRYPTION_STATE, state.value);
log.info("Responding encryption state={} success={} for keyId={}
timeMs={}",
- state.value, success, keyId, timeMs);
+ state.value, success, keyId, elapsedTimeMs(startTimeNs));
}
}
private void distributeRequest(SolrQueryRequest req, SolrQueryResponse rsp,
String keyId, long startTimeNs) {
boolean success = false;
- String collectionName = null;
State collectionState = null;
long timeAllowedMs = req.getParams().getLong(TIME_ALLOWED, 0);
- long maxTimeNs = timeAllowedMs <= 0 ? Long.MAX_VALUE : startTimeNs +
timeAllowedMs;
+ TimeOut timeOut = timeAllowedMs <= 0 ? null : new TimeOut(timeAllowedMs,
TimeUnit.MILLISECONDS, getTimeSource());
try {
- collectionName = req.getCore().getCoreDescriptor().getCollectionName();
+ String collectionName =
req.getCore().getCoreDescriptor().getCollectionName();
if (collectionName == null) {
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
"Parameter " + DISTRIB + " can only be used in Solr Cloud mode.");
}
- log.debug("Encrypt request distributed for keyId={} collection={}",
keyId, collectionName);
+ log.debug("Encrypt request distributed for keyId={}", keyId);
DocCollection docCollection =
req.getCore().getCoreContainer().getZkController().getZkStateReader().getCollection(collectionName);
if (docCollection == null) {
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
"Parameter " + DISTRIB + " present but collection '" + collectionName + "' not
found.");
}
- try (SolrClientHolder solrClient = getHttpSolrClient(req)) {
- ModifiableSolrParams params = createDistributedRequestParams(req, rsp,
keyId);
- Collection<Slice> slices = docCollection.getActiveSlices();
- Collection<Callable<State>> encryptRequests = new
ArrayList<>(slices.size());
- final String collectionNameFinal = collectionName;
- for (Slice slice : slices) {
- Replica replica = slice.getLeader();
- if (replica == null) {
- log.error("No leader found for shard {}", slice.getName());
- collectionState = State.ERROR;
- continue;
- }
- encryptRequests.add(() -> sendEncryptionRequestWithRetry(replica,
params, solrClient.getClient(), keyId, collectionNameFinal));
+ ModifiableSolrParams params = createDistributedRequestParams(req, rsp,
keyId);
+ Collection<Slice> slices = docCollection.getActiveSlices();
+ Collection<Callable<State>> encryptRequests = new
ArrayList<>(slices.size());
+ // Use the update-only http client, considering encryption is an update
as we indeed create new Lucene segments.
+ Http2SolrClient solrClient =
req.getCoreContainer().getUpdateShardHandler().getUpdateOnlyHttpClient();
Review Comment:
Seems you could move this to inside sendEncryptionRequestWithRetry, thus one
fewer param as well. It doesn't matter but sendEncryptionRequestWithRetry has
lots of params so maybe this move would be nicer. Up to you.
--
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
