diff options
author | Haakon Dybdahl <dybdahl@yahoo-inc.com> | 2017-03-09 09:50:41 +0100 |
---|---|---|
committer | Haakon Dybdahl <dybdahl@yahoo-inc.com> | 2017-03-09 09:50:41 +0100 |
commit | d976537954a8a3b63572d4b98d74a95ee35a59a1 (patch) | |
tree | 2911e9d7fa520f6879f0cf8d10dc3a189005a82a /vespa-http-client | |
parent | 3fcafa4cc9a2f29c417cfbda9bed0c8390ba2ab7 (diff) |
Improvements based on code review.
Diffstat (limited to 'vespa-http-client')
11 files changed, 164 insertions, 208 deletions
diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/Result.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/Result.java index 4488f8603c2..6b3e716c270 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/Result.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/Result.java @@ -2,9 +2,13 @@ package com.yahoo.vespa.http.client; import com.yahoo.vespa.http.client.config.Endpoint; +import com.yahoo.vespa.http.client.core.Document; import com.yahoo.vespa.http.client.core.Exceptions; import net.jcip.annotations.Immutable; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; import java.util.List; /** @@ -16,55 +20,90 @@ import java.util.List; * @since 5.1.20 */ // This should be an interface, but in order to be binary compatible during refactoring we made it abstract. -public abstract class Result { +public class Result { + public enum ResultType { + OPERATION_EXECUTED, + TRANSITIVE_ERROR, + CONDITION_NOT_MET, + FATAL_ERROR + } + + private final Document document; + private final boolean success; + private final boolean _transient; + private final List<Detail> details; + private final String localTrace; + + public Result(Document document, Collection<Detail> values, StringBuilder localTrace) { + this.document = document; + this.details = Collections.unmodifiableList(new ArrayList<>(values)); + boolean totalSuccess = true; + boolean totalTransient = true; + for (Detail d : details) { + if (d.getResultType() != ResultType.OPERATION_EXECUTED) {totalSuccess = false; } + if (d.getResultType() != ResultType.TRANSITIVE_ERROR) {totalTransient = false; } + } + this.success = totalSuccess; + this._transient = totalTransient; + this.localTrace = localTrace == null ? null : localTrace.toString(); + } + + /** * Returns the document ID that this Result is for. * * @return the document ID that this Result is for. */ - abstract public String getDocumentId(); + public String getDocumentId() { + return document.getDocumentId(); + } /** * Returns the document data. * @return data as bytebuffer. */ - abstract public CharSequence getDocumentDataAsCharSequence(); + public CharSequence getDocumentDataAsCharSequence() { + return document.getDataAsString(); + } /** * Returns the context of the object if any. * @return context. */ - abstract public Object getContext(); + public Object getContext() { + return document.getContext(); + } /** - * Returns true if the operation was successful. If at least one {@link Detail} + * Returns true if the operation(s) was successful. If at least one {@link Detail} * in {@link #getDetails()} is unsuccessful, this will return false. * * @return true if the operation was successful. */ - abstract public boolean isSuccess(); - + public boolean isSuccess() { + return success; + } /** + * @deprecated use resultType on items getDetails() to check operations. * Returns true if an error is transient, false if it is permanent. Irrelevant * if {@link #isSuccess()} is true (returns true in those cases). * * @return true if an error is transient (or there is no error), false otherwise. */ - abstract public boolean isTransient(); - - /** - * Returns true if a error was caused by condition-not-met in the document operation. - * @return true if condition is not met. - */ - abstract public boolean isConditionNotMet(); + @Deprecated + public boolean isTransient() { + return _transient; + } - abstract public List<Detail> getDetails(); + public List<Detail> getDetails() { return details; } /** * Checks if operation has been set up with local tracing. * @return true if operation has local trace. */ - abstract public boolean hasLocalTrace(); + public boolean hasLocalTrace() { + return localTrace != null; + } /** * Information in a Result for a single operation sent to a single endpoint. @@ -74,28 +113,22 @@ public abstract class Result { */ @Immutable public static final class Detail { + private final ResultType resultType; private final Endpoint endpoint; - private final boolean success; - private final boolean _transient; - private final boolean isConditionNotMet; private final Exception exception; private final String traceMessage; private final long timeStampMillis = System.currentTimeMillis(); - public Detail(Endpoint endpoint, boolean success, boolean _transient, boolean isConditionNotMet, String traceMessage, Exception e) { + public Detail(Endpoint endpoint, ResultType resultType, String traceMessage, Exception e) { this.endpoint = endpoint; - this.success = success; - this._transient = _transient; - this.isConditionNotMet = isConditionNotMet; + this.resultType = resultType; this.exception = e; this.traceMessage = traceMessage; } public Detail(Endpoint endpoint) { this.endpoint = endpoint; - this.success = true; - this._transient = true; - this.isConditionNotMet = false; + this.resultType = ResultType.OPERATION_EXECUTED; this.exception = null; this.traceMessage = null; } @@ -110,30 +143,33 @@ public abstract class Result { } /** + * @deprecated use getResultType. * Returns true if the operation was successful. * * @return true if the operation was successful. */ + @Deprecated public boolean isSuccess() { - return success; + return resultType == ResultType.OPERATION_EXECUTED; } /** + * @deprecated use getResultType. * Returns true if an error is transient, false if it is permanent. Irrelevant * if {@link #isSuccess()} is true (returns true in those cases). * * @return true if an error is transient (or there is no error), false otherwise. */ + @Deprecated public boolean isTransient() { - return _transient; + return resultType == ResultType.TRANSITIVE_ERROR || resultType == ResultType.OPERATION_EXECUTED; } /** - * Returns true if a condition in the document operation was not met. - * @return if condition not met in operation. + * Returns the result of the operation. */ - public boolean isConditionNotMet() { - return isConditionNotMet; + public ResultType getResultType() { + return resultType; } /** @@ -157,11 +193,7 @@ public abstract class Result { public String toString() { StringBuilder b = new StringBuilder(); b.append("Detail "); - b.append("success=").append(success).append(" "); - if (!success) { - b.append("transient=").append(_transient).append(" "); - b.append("conditionNotMet=").append(isConditionNotMet).append(" "); - } + b.append("resultType=").append(resultType); if (exception != null) { b.append("exception='").append(Exceptions.toMessageString(exception)).append("' "); } diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/OperationStatus.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/OperationStatus.java index d0355b0e269..cbf690c1f14 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/OperationStatus.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/OperationStatus.java @@ -8,7 +8,7 @@ import java.util.Iterator; /** - * Wrapper to represent the result of a single operation fed to Vespa. + * Serialization/deserialization class for the result of a single document operation against Vespa. * * @author <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a> * @since 5.1 @@ -26,6 +26,14 @@ public final class OperationStatus { private static final char SEPARATOR = ' '; private static final Splitter spaceSep = Splitter.on(SEPARATOR); + /** + * Constructor + * @param message some human readable information what happened + * @param operationId the doc ID for the operation + * @param errorCode if it is success, transitive, or fatal + * @param isConditionNotMet if error is due to condition not met + * @param traceMessage any tracemessage + */ public OperationStatus(String message, String operationId, ErrorCode errorCode, boolean isConditionNotMet, String traceMessage) { this.isConditionNotMet = isConditionNotMet; this.message = message; @@ -35,7 +43,7 @@ public final class OperationStatus { } /** - * Parse a single rendered OperationStatus. White space may be padded after + * Parse a single rendered OperationStatus string. White space may be padded after * and before the given status. * * @param singleLine @@ -73,6 +81,9 @@ public final class OperationStatus { return new OperationStatus(message, operationId, errorCode, isConditionNotMet, traceMessage); } + /** + * @return a string representing the status. + */ public String render() { StringBuilder s = new StringBuilder(); Encoder.encode(operationId, s).append(SEPARATOR); diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/api/ResultImpl.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/api/ResultImpl.java deleted file mode 100644 index 3e3f6897ba8..00000000000 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/api/ResultImpl.java +++ /dev/null @@ -1,94 +0,0 @@ -// Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.http.client.core.api; - -import com.yahoo.vespa.http.client.Result; -import com.yahoo.vespa.http.client.core.Document; -import net.jcip.annotations.Immutable; - -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.List; - -/** - * The result of an operation written to an OutputStream returned by - * {@link com.yahoo.vespa.http.client.Session#stream(CharSequence)}. A Result refers to a single document, - * but may contain more than one Result.Detail instances, as these pertains to a - * single endpoint, and a Result may wrap data for multiple endpoints. - * - * @author <a href="mailto:einarmr@yahoo-inc.com">Einar M R Rosenvinge</a> - * @since 5.1.20 - */ -@Immutable -final public class ResultImpl extends Result { - private final Document document; - private final boolean success; - private final boolean _transient; - private final boolean isConditionNotMet; - private final List<Detail> details; - private final String localTrace; - - public ResultImpl(Document document, Collection<Detail> values, StringBuilder localTrace) { - this.document = document; - this.details = Collections.unmodifiableList(new ArrayList<>(values)); - boolean totalSuccess = true; - boolean totalTransient = true; - boolean isConditionNotMet = true; - for (Detail d : details) { - if (!d.isSuccess()) {totalSuccess = false; } - if (!d.isTransient()) {totalTransient = false; } - if (!d.isConditionNotMet()) { isConditionNotMet = false; } - } - this.success = totalSuccess; - this._transient = totalTransient; - this.isConditionNotMet = isConditionNotMet; - this.localTrace = localTrace == null ? null : localTrace.toString(); - } - - @Override - public String getDocumentId() { - return document.getDocumentId(); - } - - @Override - public CharSequence getDocumentDataAsCharSequence() { - return document.getDataAsString(); - } - - @Override - public Object getContext() { - return document.getContext(); - } - - @Override - public boolean isSuccess() { - return success; - } - - @Override - public boolean isTransient() { - return _transient; - } - - @Override - public boolean isConditionNotMet() { return isConditionNotMet; } - - - @Override - public List<Detail> getDetails() { return details; } - - @Override - public boolean hasLocalTrace() { - return localTrace != null; - } - - @Override - public String toString() { - StringBuilder b = new StringBuilder(); - b.append("Result for '").append(document.getDocumentId()); - if (localTrace != null) { - b.append(localTrace); - } - return b.toString(); - } -} diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/EndpointResultQueue.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/EndpointResultQueue.java index 948d2e7f865..37395f87fd8 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/EndpointResultQueue.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/EndpointResultQueue.java @@ -104,7 +104,7 @@ class EndpointResultQueue { } private synchronized void failedOperationId(String operationId, Exception exception) { - EndpointResult endpointResult = EndPointResultFactory.createError(endpoint, operationId, false, exception); + EndpointResult endpointResult = EndPointResultFactory.createError(endpoint, operationId, exception); operationProcessor.resultReceived(endpointResult, clusterId); } diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/IOThread.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/IOThread.java index 9697256fbde..a6e0c9092db 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/IOThread.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/IOThread.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.http.client.core.communication; import com.google.common.annotations.Beta; +import com.yahoo.vespa.http.client.Result; import com.yahoo.vespa.http.client.config.Endpoint; import com.yahoo.vespa.http.client.core.Document; import com.yahoo.vespa.http.client.core.operationProcessor.EndPointResultFactory; @@ -245,8 +246,7 @@ class IOThread implements Runnable, AutoCloseable { statusReceivedCounter.addAndGet(endpointResults.size()); int transientErrors = 0; for (EndpointResult endpointResult : endpointResults) { - if (! endpointResult.getDetail().isSuccess() && - endpointResult.getDetail().isTransient()) { + if (endpointResult.getDetail().getResultType() == Result.ResultType.TRANSITIVE_ERROR) { transientErrors++; } resultQueue.resultReceived(endpointResult, clusterId); @@ -404,7 +404,7 @@ class IOThread implements Runnable, AutoCloseable { for (Document document : documentQueue.removeAllDocuments()) { EndpointResult endpointResult= - EndPointResultFactory.createError(endpoint, document.getOperationId(), false, exception); + EndPointResultFactory.createError(endpoint, document.getOperationId(), exception); resultQueue.failOperation(endpointResult, clusterId); } } diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/DocumentSendInfo.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/DocumentSendInfo.java index 89c2ecc8ace..b25678d2b9d 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/DocumentSendInfo.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/DocumentSendInfo.java @@ -3,7 +3,6 @@ package com.yahoo.vespa.http.client.core.operationProcessor; import com.yahoo.vespa.http.client.Result; import com.yahoo.vespa.http.client.core.Document; -import com.yahoo.vespa.http.client.core.api.ResultImpl; import java.util.HashMap; import java.util.Map; @@ -46,7 +45,7 @@ class DocumentSendInfo { } public Result createResult() { - return new ResultImpl(document, detailByClusterId.values(), localTrace); + return new Result(document, detailByClusterId.values(), localTrace); } int incRetries(int clusterId, Result.Detail detail) { diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/EndPointResultFactory.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/EndPointResultFactory.java index b357efc82d4..8c71af69100 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/EndPointResultFactory.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/EndPointResultFactory.java @@ -41,13 +41,30 @@ public final class EndPointResultFactory { } public static EndpointResult createError( - Endpoint endpoint, String operationId, boolean isConditionNotMetError, Exception exception) { - return new EndpointResult(operationId, new Result.Detail(endpoint, false, false, isConditionNotMetError, null, exception)); + Endpoint endpoint, String operationId, Exception exception) { + return new EndpointResult(operationId, new Result.Detail( + endpoint, Result.ResultType.FATAL_ERROR, null, exception)); } public static EndpointResult createTransientError( Endpoint endpoint, String operationId, Exception exception) { - return new EndpointResult(operationId, new Result.Detail(endpoint, false, true, false, null, exception)); + return new EndpointResult(operationId, new Result.Detail( + endpoint, Result.ResultType.TRANSITIVE_ERROR, null, exception)); + } + + private static Result.ResultType replyToResultType(OperationStatus reply) { + final Result.ResultType resultType; + // The ordering below is important, e.g. if success, it is never a transient error even if isTransient is true. + if (reply.errorCode.isSuccess()) { + return Result.ResultType.OPERATION_EXECUTED; + } + if (reply.isConditionNotMet) { + return Result.ResultType.CONDITION_NOT_MET; + } + if (reply.errorCode.isTransient()) { + return Result.ResultType.TRANSITIVE_ERROR; + } + return Result.ResultType.FATAL_ERROR; } private static EndpointResult parseResult(String line, Endpoint endpoint) { @@ -69,9 +86,7 @@ public final class EndPointResultFactory { return new EndpointResult( reply.operationId, new Result.Detail(endpoint, - reply.errorCode.isSuccess(), - reply.errorCode.isTransient(), - reply.isConditionNotMet, + replyToResultType(reply), reply.traceMessage, exception)); } catch (Throwable t) { diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/OperationProcessor.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/OperationProcessor.java index 199c5992e8b..b53476d2b18 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/OperationProcessor.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/operationProcessor/OperationProcessor.java @@ -110,7 +110,7 @@ public class OperationProcessor { private boolean retriedThis(EndpointResult endpointResult, DocumentSendInfo documentSendInfo, int clusterId) { final Result.Detail detail = endpointResult.getDetail(); // If success, no retries to do. - if (detail.isSuccess()) { + if (detail.getResultType() == Result.ResultType.OPERATION_EXECUTED) { return false; } @@ -125,7 +125,8 @@ public class OperationProcessor { } // TODO: Return proper error code in structured data in next version of internal API. // Error codes from messagebus/src/cpp/messagebus/errorcode.h - boolean retryThisOperation = detail.isTransient() || + boolean retryThisOperation = + detail.getResultType() == Result.ResultType.TRANSITIVE_ERROR || exceptionMessage.contains("SEND_QUEUE_CLOSED") || exceptionMessage.contains("ILLEGAL_ROUTE") || exceptionMessage.contains("NO_SERVICES_FOR_ROUTE") || @@ -247,7 +248,6 @@ public class OperationProcessor { EndPointResultFactory.createError( eio.getEndpoint(), document.getOperationId(), - false, eio), clusterConnection.getClusterId()); } diff --git a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/V3HttpAPIMultiClusterTest.java b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/V3HttpAPIMultiClusterTest.java index e401fe5cfe6..dd12cacf296 100644 --- a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/V3HttpAPIMultiClusterTest.java +++ b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/V3HttpAPIMultiClusterTest.java @@ -19,6 +19,7 @@ import static com.yahoo.vespa.http.client.TestUtils.getResults; import static com.yahoo.vespa.http.client.V3HttpAPITest.documents; import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; /** * Only runs on screwdriver to save time! @@ -129,11 +130,7 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { assertThat(r, not(nullValue())); assertThat(r.getDetails().toString(), r.isSuccess(), is(false)); assertThat(r.getDetails().size(), is(2)); - // One of the details should be true and one false. - assertThat( - r.getDetails().toString(), - r.getDetails().get(0).isSuccess() ^ r.getDetails().get(1).isSuccess(), - is(true)); + assert(r.getDetails().get(0).getResultType() != r.getDetails().get(1).getResultType()); } assertThat(results.isEmpty(), is(true)); } @@ -224,9 +221,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -281,9 +278,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -339,10 +336,10 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isTransient(), is(true)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -398,9 +395,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -454,9 +451,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -506,11 +503,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { details.put(detail.getEndpoint(), detail); } Result.Detail failed = details.remove(Endpoint.create("localhost", serverC.getPort(), false)); - assertThat(failed.toString(), failed.isSuccess(), is(false)); - assertThat(failed.toString(), failed.isTransient(), is(true)); + assertThat(failed.getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); for (Result.Detail detail : details.values()) { - assertThat(detail.toString(), detail.isSuccess(), is(true)); - assertThat(detail.toString(), detail.isTransient(), is(true)); + assertThat(detail.getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); } } } @@ -562,9 +557,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -617,9 +612,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -665,8 +660,7 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { assertThat(r, not(nullValue())); assertThat(r.getDetails().toString(), r.isSuccess(), is(false)); for (Result.Detail detail : r.getDetails()) { - assertThat(detail.toString(), detail.isSuccess(), is(false)); - assertThat(detail.toString(), detail.isTransient(), is(true)); + assertThat(detail.getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } } } @@ -715,11 +709,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { details.put(detail.getEndpoint(), detail); } Result.Detail failed = details.remove(Endpoint.create("localhost", serverC.getPort(), false)); - assertThat(failed.toString(), failed.isSuccess(), is(false)); - assertThat(failed.toString(), failed.isTransient(), is(true)); + assertThat(failed.getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); for (Result.Detail detail : details.values()) { - assertThat(detail.toString(), detail.isSuccess(), is(true)); - assertThat(detail.toString(), detail.isTransient(), is(true)); + assertThat(detail.getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); } } } @@ -764,8 +756,7 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { assertThat(r, not(nullValue())); assertThat(r.getDetails().toString(), r.isSuccess(), is(false)); for (Result.Detail detail : r.getDetails()) { - assertThat(detail.toString(), detail.isSuccess(), is(false)); - assertThat(detail.toString(), detail.isTransient(), is(true)); + assertThat(detail.getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } } } @@ -815,9 +806,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -868,9 +859,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -929,9 +920,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(true)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(true)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); } assertThat(results.isEmpty(), is(true)); @@ -987,9 +978,9 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).isSuccess(), is(false)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.FATAL_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.FATAL_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverC.getPort(), false)).getResultType(), is(Result.ResultType.FATAL_ERROR)); } assertThat(results.isEmpty(), is(true)); } @@ -1040,8 +1031,8 @@ public class V3HttpAPIMultiClusterTest extends TestOnCiBuildingSystemOnly { for (Result.Detail detail : r.getDetails()) { details.put(detail.getEndpoint(), detail); } - assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).isSuccess(), is(false)); - assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).isSuccess(), is(true)); + assertThat(details.get(Endpoint.create("localhost", serverA.getPort(), false)).getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); + assertThat(details.get(Endpoint.create("localhost", serverB.getPort(), false)).getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); //Set B in bad state => B returns bad request. diff --git a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/V3HttpAPITest.java b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/V3HttpAPITest.java index 68e86c86929..6ca4a77dcb9 100644 --- a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/V3HttpAPITest.java +++ b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/V3HttpAPITest.java @@ -84,9 +84,10 @@ public class V3HttpAPITest extends TestOnCiBuildingSystemOnly { TestDocument document = documents.get(0); Result r = results.remove(document.getDocumentId()); assertThat(r, not(nullValue())); - assertThat(r.isConditionNotMet(), is(conditionNotMet)); + if (conditionNotMet) { + assertThat(r.getDetails().iterator().next().getResultType(), is(Result.ResultType.CONDITION_NOT_MET)); + } assertThat(r.getDetails().toString(), r.isSuccess(), is(false)); - assertThat(r.getDetails().toString(), r.isConditionNotMet(), is(conditionNotMet)); assertThat(results.isEmpty(), is(true)); } } @@ -174,7 +175,7 @@ public class V3HttpAPITest extends TestOnCiBuildingSystemOnly { Result r = results.remove(document.getDocumentId()); assertThat(r, not(nullValue())); assertThat(r.getDetails().toString(), r.isSuccess(), is(false)); - assertThat(r.isTransient(), is(true)); + assertThat(r.getDetails().iterator().next().getResultType(), is(Result.ResultType.TRANSITIVE_ERROR)); } assertThat(results.isEmpty(), is(true)); } diff --git a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/communication/IOThreadTest.java b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/communication/IOThreadTest.java index 61965fa68b1..8c90c1d4f92 100644 --- a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/communication/IOThreadTest.java +++ b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/communication/IOThreadTest.java @@ -1,6 +1,7 @@ // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.http.client.core.communication; +import com.yahoo.vespa.http.client.Result; import com.yahoo.vespa.http.client.V3HttpAPITest; import com.yahoo.vespa.http.client.core.Document; import com.yahoo.vespa.http.client.core.EndpointResult; @@ -45,10 +46,11 @@ public class IOThreadTest { doAnswer(invocation -> { EndpointResult endpointResult = (EndpointResult) invocation.getArguments()[0]; assertThat(endpointResult.getOperationId(), is(expectedDocIdFail)); - assertThat(endpointResult.getDetail().isSuccess(), is(false)); assertThat(endpointResult.getDetail().getException().toString(), containsString(expectedException)); - assertThat(endpointResult.getDetail().isTransient(), is(isTransient)); + assertThat(endpointResult.getDetail().getResultType(), is( + isTransient ? Result.ResultType.TRANSITIVE_ERROR : Result.ResultType.FATAL_ERROR)); + latch.countDown(); return null; }).when(endpointResultQueue).failOperation(anyObject(), eq(0)); @@ -56,8 +58,7 @@ public class IOThreadTest { doAnswer(invocation -> { EndpointResult endpointResult = (EndpointResult) invocation.getArguments()[0]; assertThat(endpointResult.getOperationId(), is(expectedDocIdOk)); - assertThat(endpointResult.getDetail().isSuccess(), is(true)); - assertThat(endpointResult.getDetail().isTransient(), is(isTransient)); + assertThat(endpointResult.getDetail().getResultType(), is(Result.ResultType.OPERATION_EXECUTED)); latch.countDown(); return null; }).when(endpointResultQueue).resultReceived(anyObject(), eq(0)); |