Core: Add idempotency-key-lifetime to ConfigResponse #14649
Conversation
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponse.java
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java
Show resolved
Hide resolved
|
also cc @nastra @flyrain @stevenzwu |
| } | ||
|
|
||
| /** Sets the optional idempotency key lifetime advertised by the server. */ | ||
| public Builder withIdempotencyKeyLifetime(@Nullable String lifetime) { |
There was a problem hiding this comment.
I don't think we typically add @Nullable to method params
| Duration.parse(lifetime); | ||
| builder.withIdempotencyKeyLifetime(lifetime); | ||
| } catch (RuntimeException e) { | ||
| // ignore invalid value |
There was a problem hiding this comment.
I don't think we should ignore this if the lifetime is an invalid value. Also I would add the parsing to JsonUtil to have getDurationStringOrNull(similar to getStringOrNull). That method would throw if the lifetime string can't be parsed to a string and would throw a Cannot parse to a duration string value: ...
There was a problem hiding this comment.
Fixed the code to throw exception.
| + " \"overrides\" : { },\n" | ||
| + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n" | ||
| + "}"); | ||
| // invalid value is treated as "not advertised" |
There was a problem hiding this comment.
I believe an invalid value should always throw, similar to how e.g. an invalid value for a number throws
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java
Outdated
Show resolved
Hide resolved
| java.time.Duration.parse(value); | ||
| } catch (RuntimeException e) { | ||
| throw new IllegalArgumentException( | ||
| String.format("Cannot parse to a duration string value: %s: %s", property, value), e); |
There was a problem hiding this comment.
| String.format("Cannot parse to a duration string value: %s: %s", property, value), e); | |
| String.format("Cannot parse to a duration string value: %s: %s", property, node.get(property), e); |
for consistency with other JsonUtil methods we should probably add the actual JSON node into the error msg
| JsonUtil.getDurationStringOrNull( | ||
| "x", JsonUtil.mapper().readTree("{\"x\": \"30M\"}"))) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("x: 30M"); |
There was a problem hiding this comment.
| .hasMessageContaining("x: 30M"); | |
| .hasMessage("Cannot parse to a duration string value: x: \"30M\""); |
There was a problem hiding this comment.
I believe it's better to verify the exact error msg as we do in other tests in order to make sure that the error msg has the right format/wording
| () -> | ||
| JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"\"}"))) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("x: "); |
There was a problem hiding this comment.
| .hasMessageContaining("x: "); | |
| .hasMessage("Cannot parse to a duration string value: x: \"\""); |
| + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n" | ||
| + "}")) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("idempotency-key-lifetime: not-a-duration"); |
There was a problem hiding this comment.
| .hasMessageContaining("idempotency-key-lifetime: not-a-duration"); | |
| .hasMessage( | |
| "Cannot parse to a duration string value: idempotency-key-lifetime: \"not-a-duration\""); |
| + " \"idempotency-key-lifetime\" : \"\"\n" | ||
| + "}")) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("idempotency-key-lifetime: "); |
There was a problem hiding this comment.
| .hasMessageContaining("idempotency-key-lifetime: "); | |
| .hasMessage("Cannot parse to a duration string value: idempotency-key-lifetime: \"\""); |
|
|
||
| if (json.hasNonNull(IDEMPOTENCY_KEY_LIFETIME)) { | ||
| String lifetime = JsonUtil.getDurationStringOrNull(IDEMPOTENCY_KEY_LIFETIME, json); | ||
| if (lifetime != null) { |
There was a problem hiding this comment.
I don't think we need the additional null check here, since the Builder can just pass through what was set, so this could be simplified/inlined to builder.withIdempotencyKeyLifetime(JsonUtil.getDurationStringOrNull(IDEMPOTENCY_KEY_LIFETIME, json));
nastra
changed the title
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM too, thanks @huaxingao !
|
Thanks @huaxingao for the change and @nastra for the review ! |
|
Thanks a lot @singhpk234 @nastra |
3 participants
OpenAPI defines
idempotency-key-lifetimein CatalogConfig. This PR surfaces that field in the Java model and JSON parser/serializer so capability can be discovered. No client/server behavior changes in this PR.Follow-ups (separate PRs)