You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "huaxingao (via GitHub)" <gi...@apache.org> on 2025/11/21 04:15:38 UTC
[PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
huaxingao opened a new pull request, #14649:
URL: https://github.com/apache/iceberg/pull/14649
OpenAPI defines `idempotency-key-lifetime` in 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)
- Client utilities (UUIDv7, constants).
- Client opt-in wiring in RESTSessionCatalog and mutation calls.
- E2E tests with test server idempotency layer.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2557186043
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,49 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot parse to a duration string value: idempotency-key-lifetime");
+ }
+
+ @Test
+ public void emptyIdempotencyLifetime() {
Review Comment:
Combined the two tests
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#issuecomment-3569057862
also cc @nastra @flyrain @stevenzwu
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add idempotency-key-lifetime to ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2561081036
##########
core/src/main/java/org/apache/iceberg/util/JsonUtil.java:
##########
@@ -189,6 +189,22 @@ public static String getStringOrNull(String property, JsonNode node) {
return getString(property, node);
}
+ public static String getDurationStringOrNull(String property, JsonNode node) {
+ String value = getStringOrNull(property, node);
+ if (value == null) {
+ return null;
+ }
+
+ try {
+ 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);
Review Comment:
Done! Thanks!
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -156,6 +156,29 @@ public void getStringOrNull() throws JsonProcessingException {
.hasMessage("Cannot parse to a string value: x: 23");
}
+ @Test
+ public void getDurationStringOrNull() throws JsonProcessingException {
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ .isNull();
+ assertThat(
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"PT30M\"}")))
+ .isEqualTo("PT30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull(
+ "x", JsonUtil.mapper().readTree("{\"x\": \"30M\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: 30M");
Review Comment:
Done
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2559041784
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -156,6 +156,29 @@ public void getStringOrNull() throws JsonProcessingException {
.hasMessage("Cannot parse to a string value: x: 23");
}
+ @Test
+ public void getDurationStringOrNull() throws JsonProcessingException {
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ .isNull();
+ assertThat(
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"PT30M\"}")))
+ .isEqualTo("PT30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull(
+ "x", JsonUtil.mapper().readTree("{\"x\": \"30M\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: 30M");
Review 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
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2559063619
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java:
##########
@@ -83,6 +88,13 @@ public static ConfigResponse fromJson(JsonNode json) {
.collect(Collectors.toList()));
}
+ if (json.hasNonNull(IDEMPOTENCY_KEY_LIFETIME)) {
+ String lifetime = JsonUtil.getDurationStringOrNull(IDEMPOTENCY_KEY_LIFETIME, json);
+ if (lifetime != null) {
Review 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));`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add idempotency-key-lifetime to ConfigResponse [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 merged PR #14649:
URL: https://github.com/apache/iceberg/pull/14649
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2557187888
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,49 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot parse to a duration string value: idempotency-key-lifetime");
+ }
+
+ @Test
+ public void emptyIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot parse to a duration string value: idempotency-key-lifetime");
Review Comment:
I changed to ` .hasMessageContaining("idempotency-key-lifetime: not-a-duration");`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add idempotency-key-lifetime to ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2561081919
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -156,6 +156,29 @@ public void getStringOrNull() throws JsonProcessingException {
.hasMessage("Cannot parse to a string value: x: 23");
}
+ @Test
+ public void getDurationStringOrNull() throws JsonProcessingException {
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ .isNull();
+ assertThat(
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"PT30M\"}")))
+ .isEqualTo("PT30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull(
+ "x", JsonUtil.mapper().readTree("{\"x\": \"30M\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: 30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: ");
Review Comment:
Done. Thanks!
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,46 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("idempotency-key-lifetime: not-a-duration");
Review Comment:
Fixed. Thanks
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,46 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("idempotency-key-lifetime: not-a-duration");
+
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("idempotency-key-lifetime: ");
Review Comment:
Fixed
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2554792265
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponse.java:
##########
@@ -178,8 +199,14 @@ public Builder withEndpoints(List<Endpoint> endpointsToAdd) {
return this;
}
+ /** Sets the optional idempotency key lifetime advertised by the server. */
+ public Builder withIdempotencyKeyLifetime(@Nullable String lifetime) {
Review Comment:
I don't think we typically add `@Nullable` to method params
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2559035803
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -156,6 +156,29 @@ public void getStringOrNull() throws JsonProcessingException {
.hasMessage("Cannot parse to a string value: x: 23");
}
+ @Test
+ public void getDurationStringOrNull() throws JsonProcessingException {
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ .isNull();
+ assertThat(
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"PT30M\"}")))
+ .isEqualTo("PT30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull(
+ "x", JsonUtil.mapper().readTree("{\"x\": \"30M\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: 30M");
Review Comment:
```suggestion
.hasMessage("Cannot parse to a duration string value: x: \"30M\"");
```
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -156,6 +156,29 @@ public void getStringOrNull() throws JsonProcessingException {
.hasMessage("Cannot parse to a string value: x: 23");
}
+ @Test
+ public void getDurationStringOrNull() throws JsonProcessingException {
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ .isNull();
+ assertThat(
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"PT30M\"}")))
+ .isEqualTo("PT30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull(
+ "x", JsonUtil.mapper().readTree("{\"x\": \"30M\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: 30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: ");
Review Comment:
```suggestion
.hasMessage("Cannot parse to a duration string value: x: \"\"");
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2559051068
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,46 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("idempotency-key-lifetime: not-a-duration");
+
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("idempotency-key-lifetime: ");
Review Comment:
```suggestion
.hasMessage("Cannot parse to a duration string value: idempotency-key-lifetime: \"\"");
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2553708392
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java:
##########
@@ -83,6 +88,10 @@ public static ConfigResponse fromJson(JsonNode json) {
.collect(Collectors.toList()));
}
+ if (json.hasNonNull(IDEMPOTENCY_KEY_LIFETIME)) {
Review Comment:
Added a check and a test
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2555067593
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,49 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot parse to a duration string value: idempotency-key-lifetime");
Review Comment:
shouldn't this be showing `not-a-duration` in the error msg instead of `idempotency-key-lifetime`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2559072924
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java:
##########
@@ -83,6 +88,10 @@ public static ConfigResponse fromJson(JsonNode json) {
.collect(Collectors.toList()));
}
+ if (json.hasNonNull(IDEMPOTENCY_KEY_LIFETIME)) {
Review Comment:
the String emptiness check should be part of `getDurationStringOrNull`. So the current version of the code makes sense to me
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2553708236
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponse.java:
##########
@@ -178,8 +199,14 @@ public Builder withEndpoints(List<Endpoint> endpointsToAdd) {
return this;
}
+ /** Sets the optional idempotency key lifetime advertised by the server. */
+ public Builder withIdempotencyKeyLifetime(@Nullable String lifetime) {
+ this.idempotencyKeyLifetime = lifetime;
Review Comment:
I added a check in the parser and also added a neg test.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2559034625
##########
core/src/main/java/org/apache/iceberg/util/JsonUtil.java:
##########
@@ -189,6 +189,22 @@ public static String getStringOrNull(String property, JsonNode node) {
return getString(property, node);
}
+ public static String getDurationStringOrNull(String property, JsonNode node) {
+ String value = getStringOrNull(property, node);
+ if (value == null) {
+ return null;
+ }
+
+ try {
+ 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);
Review Comment:
```suggestion
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
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -156,6 +156,29 @@ public void getStringOrNull() throws JsonProcessingException {
.hasMessage("Cannot parse to a string value: x: 23");
}
+ @Test
+ public void getDurationStringOrNull() throws JsonProcessingException {
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ .isNull();
+ assertThat(
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"PT30M\"}")))
+ .isEqualTo("PT30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull(
+ "x", JsonUtil.mapper().readTree("{\"x\": \"30M\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: 30M");
Review Comment:
```suggestion
.hasMessageContaining("Cannot parse to a duration string value: x: \"30M\"");
```
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -156,6 +156,29 @@ public void getStringOrNull() throws JsonProcessingException {
.hasMessage("Cannot parse to a string value: x: 23");
}
+ @Test
+ public void getDurationStringOrNull() throws JsonProcessingException {
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ .isNull();
+ assertThat(
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"PT30M\"}")))
+ .isEqualTo("PT30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull(
+ "x", JsonUtil.mapper().readTree("{\"x\": \"30M\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: 30M");
+
+ assertThatThrownBy(
+ () ->
+ JsonUtil.getDurationStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"\"}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("x: ");
Review Comment:
```suggestion
.hasMessageContaining("Cannot parse to a duration string value: x: \"\"");
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2559049519
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,46 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("idempotency-key-lifetime: not-a-duration");
Review Comment:
```suggestion
.hasMessage(
"Cannot parse to a duration string value: idempotency-key-lifetime: \"not-a-duration\"");
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2559063619
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java:
##########
@@ -83,6 +88,13 @@ public static ConfigResponse fromJson(JsonNode json) {
.collect(Collectors.toList()));
}
+ if (json.hasNonNull(IDEMPOTENCY_KEY_LIFETIME)) {
+ String lifetime = JsonUtil.getDurationStringOrNull(IDEMPOTENCY_KEY_LIFETIME, json);
+ if (lifetime != null) {
Review Comment:
I don't think we need the additional null check here, since the Builder can just pass through what was set
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2554807540
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,47 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ ConfigResponse parsed =
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}");
+ // invalid value is treated as "not advertised"
Review Comment:
I believe an invalid value should always throw, similar to how e.g. an invalid value for a number throws
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2554804188
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java:
##########
@@ -83,6 +89,19 @@ public static ConfigResponse fromJson(JsonNode json) {
.collect(Collectors.toList()));
}
+ if (json.hasNonNull(IDEMPOTENCY_KEY_LIFETIME)) {
+ String lifetime = JsonUtil.getString(IDEMPOTENCY_KEY_LIFETIME, json);
+ // empty or invalid -> treated as not advertised (skip setting)
+ if (lifetime != null && !lifetime.trim().isEmpty()) {
+ try {
+ Duration.parse(lifetime);
+ builder.withIdempotencyKeyLifetime(lifetime);
+ } catch (RuntimeException e) {
+ // ignore invalid value
Review 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: ...`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2557184918
##########
core/src/main/java/org/apache/iceberg/util/JsonUtil.java:
##########
@@ -189,6 +189,22 @@ public static String getStringOrNull(String property, JsonNode node) {
return getString(property, node);
}
+ public static String getDurationStringOrNull(String property, JsonNode node) {
Review Comment:
Added a test in `TestJsonUtil`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add idempotency-key-lifetime to ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#issuecomment-3577955463
Thanks a lot @singhpk234 @nastra
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add idempotency-key-lifetime to ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2561082640
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java:
##########
@@ -83,6 +88,13 @@ public static ConfigResponse fromJson(JsonNode json) {
.collect(Collectors.toList()));
}
+ if (json.hasNonNull(IDEMPOTENCY_KEY_LIFETIME)) {
+ String lifetime = JsonUtil.getDurationStringOrNull(IDEMPOTENCY_KEY_LIFETIME, json);
+ if (lifetime != null) {
Review Comment:
Fixed. Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2548581529
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java:
##########
@@ -83,6 +88,10 @@ public static ConfigResponse fromJson(JsonNode json) {
.collect(Collectors.toList()));
}
+ if (json.hasNonNull(IDEMPOTENCY_KEY_LIFETIME)) {
Review Comment:
should we check of "" string too ? i guess post ISO string validation this would not be required
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2554945907
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponse.java:
##########
@@ -178,8 +199,14 @@ public Builder withEndpoints(List<Endpoint> endpointsToAdd) {
return this;
}
+ /** Sets the optional idempotency key lifetime advertised by the server. */
+ public Builder withIdempotencyKeyLifetime(@Nullable String lifetime) {
Review Comment:
Removed. Thanks
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2555061483
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,49 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot parse to a duration string value: idempotency-key-lifetime");
+ }
+
+ @Test
+ public void emptyIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot parse to a duration string value: idempotency-key-lifetime");
Review Comment:
I don't think `idempotency-key-lifetime` is correct here. The error msg should actually contain the value from the JSON node, which in this case would be an empty string
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2555051503
##########
core/src/main/java/org/apache/iceberg/util/JsonUtil.java:
##########
@@ -189,6 +189,22 @@ public static String getStringOrNull(String property, JsonNode node) {
return getString(property, node);
}
+ public static String getDurationStringOrNull(String property, JsonNode node) {
Review Comment:
nit: for completeness you might also want to add a test to `TestJsonUtil` for this
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2548578317
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponse.java:
##########
@@ -178,8 +199,14 @@ public Builder withEndpoints(List<Endpoint> endpointsToAdd) {
return this;
}
+ /** Sets the optional idempotency key lifetime advertised by the server. */
+ public Builder withIdempotencyKeyLifetime(@Nullable String lifetime) {
+ this.idempotencyKeyLifetime = lifetime;
Review Comment:
should we check lifetime string is an `ISO-8601 duration string` ? and add neg case where lifetime returned from server is not a valid string ?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2554951938
##########
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java:
##########
@@ -83,6 +89,19 @@ public static ConfigResponse fromJson(JsonNode json) {
.collect(Collectors.toList()));
}
+ if (json.hasNonNull(IDEMPOTENCY_KEY_LIFETIME)) {
+ String lifetime = JsonUtil.getString(IDEMPOTENCY_KEY_LIFETIME, json);
+ // empty or invalid -> treated as not advertised (skip setting)
+ if (lifetime != null && !lifetime.trim().isEmpty()) {
+ try {
+ Duration.parse(lifetime);
+ builder.withIdempotencyKeyLifetime(lifetime);
+ } catch (RuntimeException e) {
+ // ignore invalid value
Review Comment:
Fixed the code to throw exception.
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,47 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ ConfigResponse parsed =
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}");
+ // invalid value is treated as "not advertised"
Review Comment:
Fixed. Thanks
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2555054671
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,49 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot parse to a duration string value: idempotency-key-lifetime");
+ }
+
+ @Test
+ public void emptyIdempotencyLifetime() {
Review Comment:
nit: this could be tested inside `invalidIdempotencyLifetime` since it's also invalid
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Surface idempotency-key-lifetime in ConfigResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#discussion_r2557188581
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java:
##########
@@ -214,4 +214,49 @@ public void roundTripSerdeWithEndpoints() {
assertThat(ConfigResponseParser.toJson(ConfigResponseParser.fromJson(json), true))
.isEqualTo(expectedJson);
}
+
+ @Test
+ public void idempotencyLifetimeOnly() {
+ ConfigResponse response = ConfigResponse.builder().withIdempotencyKeyLifetime("PT30M").build();
+
+ String expectedJson =
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"PT30M\"\n"
+ + "}";
+
+ String json = ConfigResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+ ConfigResponse roundTripped = ConfigResponseParser.fromJson(json);
+ assertThat(roundTripped.idempotencyKeyLifetime()).isEqualTo("PT30M");
+ }
+
+ @Test
+ public void invalidIdempotencyLifetime() {
+ assertThatThrownBy(
+ () ->
+ ConfigResponseParser.fromJson(
+ "{\n"
+ + " \"defaults\" : { },\n"
+ + " \"overrides\" : { },\n"
+ + " \"idempotency-key-lifetime\" : \"not-a-duration\"\n"
+ + "}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot parse to a duration string value: idempotency-key-lifetime");
Review Comment:
Changed to `.hasMessageContaining("idempotency-key-lifetime: ");`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add idempotency-key-lifetime to ConfigResponse [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #14649:
URL: https://github.com/apache/iceberg/pull/14649#issuecomment-3577952018
Thanks @huaxingao for the change and @nastra for the review !
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org