Renamed EncodedValue to EncodedValues to support multiple details #164

mfateev · 2020-07-31T01:37:20Z

Updated DataConverter API to support extracting individual values from the serialized list.
Renamed EncodedValue to EncodedValues.
Added ApplicationFailure factories for clarity.

mastermanu · 2020-07-31T01:46:15Z

src/main/java/io/temporal/common/converter/DataConverter.java

+      Type[] genericParameterTypes)
+      throws DataConverterException {
+    if (parameterTypes != null
+        && (parameterTypes == null || parameterTypes.length != genericParameterTypes.length)) {


the first clause of this condition checks that parameterTypes is not null, so this parameterTypes==null check is unnecessary, right? Or did you mean to check if genericParameterTypes is null?

good catch!

mastermanu · 2020-07-31T01:50:28Z

src/main/java/io/temporal/common/converter/DataConverter.java

+    for (int i = 0; i < parameterTypes.length; i++) {
+      Class<?> pt = parameterTypes[i];
+      Type gt = genericParameterTypes[i];
+      if (i >= count) {


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?

Good question. The current answer is that it is needed to be able to add and remove arguments of functions.

mastermanu · 2020-07-31T01:56:07Z

src/main/java/io/temporal/common/converter/EncodedValues.java

    } else {
-      if (converter == null) {
-        throw new IllegalStateException("converter not set");
+      if (payloads.isPresent()) {


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?

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.

mastermanu · 2020-07-31T01:59:13Z

src/main/java/io/temporal/common/converter/EncodedValues.java

+      if (converter == null) {
+        throw new IllegalStateException("converter not set");
+      }
+      return converter.fromPayloads(index, payloads, parameterType, genericParameterType);


No need to initialize / cache the converted object into the values array right?

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.

mastermanu · 2020-07-31T02:01:46Z

src/main/java/io/temporal/failure/ApplicationFailure.java

  private boolean nonRetryable;

  /**
+   * New ApplicationFailure with {@link #isNonRetryable()} flag set to false. Note that this


didn't quite follow the 2nd sentence of this comment. did you mean to say if the type is NOT included

Good catch.

mastermanu · 2020-07-31T02:03:09Z

src/main/java/io/temporal/failure/ApplicationFailure.java

   */
-  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) {


why not just call it newRetryableFailure?

I called it initially. But then I found that it is very confusing that it can be not retried if the policy says so.

mastermanu · 2020-07-31T02:03:51Z

src/main/java/io/temporal/failure/ApplicationFailure.java

-  public ApplicationFailure(String message) {
-    this(message, null);
+  static ApplicationFailure newFromValues(
+      String message, String type, boolean nonRetryable, Values details, Throwable cause) {


[Nit], having boolean names that are negative (nonRetryable vs retryable) can be kind of confusing

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.

mastermanu · 2020-07-31T02:05:24Z

src/main/java/io/temporal/internal/sync/ActivityExecutionContextImpl.java

      }
      Optional<Payloads> details = info.getHeartbeatDetails();
-      return Optional.ofNullable(dataConverter.fromPayloads(details, detailsClass, detailsType));
+      return Optional.ofNullable(dataConverter.fromPayloads(0, details, detailsClass, detailsType));


we didn't want this function to have an overload without the index that internally assumes the first element? seems painful to pass in 0 every time

This class is used only internally by the SDK, so I think having an explicit index makes sense to indicate that it can hold multiple values.

mastermanu · 2020-07-31T02:08:12Z

src/test/java/io/temporal/workflow/WorkflowTest.java

      assertEquals("foo", ((ApplicationFailure) e.getCause()).getType());
-      assertEquals("details", ((ApplicationFailure) e.getCause()).getDetails().get(String.class));
+      assertEquals(
+          "details1", ((ApplicationFailure) e.getCause()).getDetails().get(0, String.class));


and we still have a test that validates get without the index specified works fine?

I checked and found at least two unit tests that call it.

… details

mastermanu · 2020-07-31T02:12:42Z

src/main/java/io/temporal/common/converter/EncodedValues.java

-    return converter.fromPayloads(payloads, parameterType, genericParameterType);
+    if (values != null) {
+      @SuppressWarnings("unchecked")
+      T result = (T) values[index];


if the user exceeds the bounds of the values/payload, no issues right?

multiple details

e39d1fb

mfateev requested a review from mastermanu July 31, 2020 01:37

mastermanu approved these changes Jul 31, 2020

View reviewed changes

Merge branch 'master' of github.com:temporalio/temporal-java-sdk into…

26f60fb

… details

mastermanu reviewed Jul 31, 2020

View reviewed changes

mfateev added 3 commits July 30, 2020 19:37

PR comments

e1a54cd

PR comments

2aaaaf8

proto updated

398082f

mastermanu approved these changes Jul 31, 2020

View reviewed changes

mfateev merged commit 19c701d into temporalio:master Jul 31, 2020

mfateev deleted the details branch July 31, 2020 02:49

Renamed EncodedValue to EncodedValues to support multiple details #164

Renamed EncodedValue to EncodedValues to support multiple details #164

Uh oh!

Conversation

mfateev commented Jul 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mastermanu Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mastermanu Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mastermanu Jul 31, 2020 •

edited

Loading

mastermanu Jul 31, 2020 •

edited

Loading