-
Notifications
You must be signed in to change notification settings - Fork 180
Renamed EncodedValue to EncodedValues to support multiple details #164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,30 +24,30 @@ | |
| import java.util.Objects; | ||
| import java.util.Optional; | ||
|
|
||
| public final class EncodedValue implements Value { | ||
| public final class EncodedValues implements Values { | ||
| private Optional<Payloads> payloads; | ||
| private DataConverter converter; | ||
| private final Optional<Object> value; | ||
| private Object[] values; | ||
|
|
||
| public EncodedValue(Optional<Payloads> payloads, DataConverter converter) { | ||
| public EncodedValues(Optional<Payloads> payloads, DataConverter converter) { | ||
| this.payloads = Objects.requireNonNull(payloads); | ||
| this.converter = converter; | ||
| this.value = null; | ||
| this.values = null; | ||
| } | ||
|
|
||
| public <T> EncodedValue(T value) { | ||
| this.value = Optional.ofNullable(value); | ||
| public EncodedValues(Object... values) { | ||
| this.values = values; | ||
| this.payloads = null; | ||
| } | ||
|
|
||
| public Optional<Payloads> toPayloads() { | ||
| if (payloads == null) { | ||
| if (!value.isPresent()) { | ||
| if (values == null || values.length == 0) { | ||
| payloads = Optional.empty(); | ||
| } else if (converter == null) { | ||
| throw new IllegalStateException("converter not set"); | ||
| } else { | ||
| payloads = converter.toPayloads(value.get()); | ||
| payloads = converter.toPayloads(values); | ||
| } | ||
| } | ||
| return payloads; | ||
|
|
@@ -58,22 +58,35 @@ public void setDataConverter(DataConverter converter) { | |
| } | ||
|
|
||
| @Override | ||
| public <T> T get(Class<T> parameterType) throws DataConverterException { | ||
| if (value != null) { | ||
| @SuppressWarnings("unchecked") | ||
| T result = (T) value.orElse(null); | ||
| return result; | ||
| public int getSize() { | ||
| if (values != null) { | ||
| return values.length; | ||
| } else { | ||
| if (converter == null) { | ||
| throw new IllegalStateException("converter not set"); | ||
| if (payloads.isPresent()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we are guaranteed that at least one of values / payloads is not null right? Basically, values and payloads cannot both be null because (Object... values) guarantees values is at least of length 0 if that constructor is used?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a lot of cases, Payloads are not set at all if there are no any values. But we probably need to audit for this. |
||
| return payloads.get().getPayloadsCount(); | ||
| } else { | ||
| return 0; | ||
| } | ||
| return converter.fromPayloads(payloads, parameterType, parameterType); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public <T> T get(Class<T> parameterType, Type genericParameterType) | ||
| public <T> T get(int index, Class<T> parameterType) throws DataConverterException { | ||
| return get(index, parameterType, parameterType); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> T get(int index, Class<T> parameterType, Type genericParameterType) | ||
| throws DataConverterException { | ||
| return converter.fromPayloads(payloads, parameterType, genericParameterType); | ||
| if (values != null) { | ||
| @SuppressWarnings("unchecked") | ||
| T result = (T) values[index]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the user exceeds the bounds of the values/payload, no issues right? |
||
| return result; | ||
| } else { | ||
| if (converter == null) { | ||
| throw new IllegalStateException("converter not set"); | ||
| } | ||
| return converter.fromPayloads(index, payloads, parameterType, genericParameterType); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to initialize / cache the converted object into the values array right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea of Values is that it can be created with objects or Payloads. I'm not sure if caching is really going to help here as every value accessed only once I believe. |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,8 +21,8 @@ | |
|
|
||
| import com.google.common.base.Strings; | ||
| import io.temporal.common.converter.DataConverter; | ||
| import io.temporal.common.converter.EncodedValue; | ||
| import io.temporal.common.converter.Value; | ||
| import io.temporal.common.converter.EncodedValues; | ||
| import io.temporal.common.converter.Values; | ||
|
|
||
| /** | ||
| * Application failure is used to communicate application specific failures between workflows and | ||
|
|
@@ -51,49 +51,47 @@ | |
| */ | ||
| public final class ApplicationFailure extends TemporalFailure { | ||
| private final String type; | ||
| private final Value details; | ||
| private final Values details; | ||
| private boolean nonRetryable; | ||
|
|
||
| /** | ||
| * New ApplicationFailure with {@link #isNonRetryable()} flag set to false. Note that this | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. didn't quite follow the 2nd sentence of this comment. did you mean to say if the type is NOT included
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. |
||
| * exception still can be not retried by the service if its type is included into doNotRetry | ||
| * property of the correspondent retry policy. | ||
| * | ||
| * @param message optional error message | ||
| * @param type optional error type that is used by {@link | ||
| * io.temporal.common.RetryOptions#addDoNotRetry(String...)}. | ||
| * @param details optional details about the failure. They are serialized using the same approach | ||
| * as arguments and results and can be accessed through {@link #getDetails()} | ||
| * @param cause failure cause. Each element of the cause chain is converted to ApplicationFailure | ||
| * if it doesn't extend {@link TemporalFailure}. | ||
| * as arguments and results. | ||
| */ | ||
| public ApplicationFailure(String message, String type, Object details, Exception cause) { | ||
| this(message, type, new EncodedValue(details), false, cause); | ||
| public static ApplicationFailure newFailure(String message, String type, Object... details) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just call it newRetryableFailure?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I called it initially. But then I found that it is very confusing that it can be not retried if the policy says so. |
||
| return new ApplicationFailure(message, type, false, new EncodedValues(details), null); | ||
| } | ||
|
|
||
| /** | ||
| * New ApplicationFailure with {@link #isNonRetryable()} flag set to true. | ||
| * | ||
| * <p>It means that this exception is not going to be retried even if it is not included into | ||
| * retry policy doNotRetry list. | ||
| * | ||
| * @param message optional error message | ||
| * @param type optional error type that is used by {@link | ||
| * io.temporal.common.RetryOptions#addDoNotRetry(String...)}. | ||
| * @param type optional error type | ||
| * @param details optional details about the failure. They are serialized using the same approach | ||
| * as arguments and results. | ||
| */ | ||
| public ApplicationFailure(String message, String type, Object details) { | ||
| this(message, type, new EncodedValue(details), false, null); | ||
| } | ||
|
|
||
| /** | ||
| * @param message optional error message | ||
| * @param type optional error type that is used by {@link | ||
| * io.temporal.common.RetryOptions#addDoNotRetry(String...)}. | ||
| */ | ||
| public ApplicationFailure(String message, String type) { | ||
| this(message, type, new EncodedValue(null), false, null); | ||
| public static ApplicationFailure newNonRetryableFailure( | ||
| String message, String type, Object... details) { | ||
| return new ApplicationFailure(message, type, true, new EncodedValues(details), null); | ||
| } | ||
|
|
||
| /** * @param message optional error message */ | ||
| public ApplicationFailure(String message) { | ||
| this(message, null); | ||
| static ApplicationFailure newFromValues( | ||
| String message, String type, boolean nonRetryable, Values details, Throwable cause) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Nit], having boolean names that are negative (nonRetryable vs retryable) can be kind of confusing
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I almost always follow the pattern that the default value should be the default value for the type. So the default of the boolean type is false. So the field is called nonRetryable as its default is false. But I agree that it looks weird in the parameter name. |
||
| return new ApplicationFailure(message, type, nonRetryable, details, cause); | ||
| } | ||
|
|
||
| ApplicationFailure( | ||
| String message, String type, Value details, boolean nonRetryable, Exception cause) { | ||
| String message, String type, boolean nonRetryable, Values details, Throwable cause) { | ||
| super(getMessage(message, type, nonRetryable), message, cause); | ||
| this.type = type; | ||
| this.details = details; | ||
|
|
@@ -104,7 +102,7 @@ public String getType() { | |
| return type; | ||
| } | ||
|
|
||
| public Value getDetails() { | ||
| public Values getDetails() { | ||
| return details; | ||
| } | ||
|
|
||
|
|
@@ -118,7 +116,7 @@ public boolean isNonRetryable() { | |
|
|
||
| @Override | ||
| public void setDataConverter(DataConverter converter) { | ||
| ((EncodedValue) details).setDataConverter(converter); | ||
| ((EncodedValues) details).setDataConverter(converter); | ||
| } | ||
|
|
||
| private static String getMessage(String message, String type, boolean nonRetryable) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the count of payloads is much greater than that of the parameters? Basically does the user need to know that they are missing out on some payloads? Or is this well understood?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. The current answer is that it is needed to be able to add and remove arguments of functions.