You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "gaborkaszab (via GitHub)" <gi...@apache.org> on 2025/09/09 09:33:06 UTC
[PR] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
gaborkaszab opened a new pull request, #14035:
URL: https://github.com/apache/iceberg/pull/14035
(no comment)
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2377137224
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
Sorry for the delay, @gaborkaszab @pvary .
So @pvary, I do think it's like point 1 where it's a reference implementation of showing how things should work but i'm also aiming for "principle of least surprise" here. Also I just am a little bit against exposing concepts like exceptions for representing cases where it's not really exceptional. At the end of the day the 304 is really the server telling the client "hey use your cache".
Separating out the client side and server side:
1.) On the client side, after looking a bit further, clients like the Github cli and other Http client frameworks do essentially handle null response bodies for 304 and it's expected that the response body is null for a 304.
2.) On the server side, it sounds like the reason for adding the exception was that our current implementation of `RESTCatalogAdapter` doesn't have a way of expressing non-200 codes without an exception? I feel like that's the limitation to address in `RESTCatalogAdapter`.
In the end I'm mostly +0 if we want to do the exception mostly because it just feels like exposing an unnecessary concept to work around the current reference server handling, and that can lead to people misusing that exception since it's in the public API. It'd be ideal if we can at least show how complicated the other solution is for being able to return status codes other than 200 without an exception.
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2388453206
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
Thank you @gaborkaszab for implementing the alternative, I took a look and that's much more inline with what I was thinking and I think I'd prefer that solution.
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2358518093
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
@amogh-jahagirdar: How do we look at the RESTCatalogAdapter?
1. Is this just a reference implementation which show how should things work, and which could be used for testing the REST Client, or
2. Is it supposed to be a production ready code?
I'm asking, because for the 1st we need the simplest, most readable code. In this case I would prefer the `NotModifiedException` which clearly describes what is happening. If we aim for production code, I would prefer to avoid handling typical cases with exceptions. In this case, I would probably prefer returning a Pair<Changed, Table>, or some other specific object based solution.
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2377137785
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
Sorry for the delay, @gaborkaszab @pvary .
So @pvary, I do think it's like point 1 where it's a reference implementation of showing how things should work but i'm also aiming for "principle of least surprise" here. Also I just am a little bit against exposing concepts like exceptions for representing cases where it's not really exceptional. At the end of the day the 304 is really the server telling the client "hey use your cache".
Separating out the client side and server side:
1.) On the client side, after looking a bit further, clients like the Github cli and other Http client frameworks do essentially handle null response bodies for 304 and it's expected that the response body is null for a 304.
2.) On the server side, it sounds like the reason for adding the exception was that our current implementation of `RESTCatalogAdapter` doesn't have a way of expressing non-200 codes without an exception? I feel like that's the limitation to address in `RESTCatalogAdapter`.
In the end I'm mostly +0 if we want to do the exception mostly because it just feels like exposing an unnecessary concept to work around the current reference server handling, and that can lead to people misusing that exception since it's in the public API. It'd be ideal if we can at least show how complicated the other solution is for being able to return status codes other than 200 without an exception.
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2382688591
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
@amogh-jahagirdar FYI, [this is](https://github.com/apache/iceberg/commit/bc704dad33b44a942bb264409c0bce027c76256f) how it would look like to return null from `RESTCatalogAdapter`. I think this is a reasonable alternative to the exception driven approach. I don't think we have any other way to return 304 because of the scenario 2) I described in my previous comment. WDYT?
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2335196208
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
OK I guess the argument against doing it in the http client is that if we just return null, it's a bit weird for clients to now have to worry about if (loadTableResponse != null) if it's a 304 vs getting an explicit exception and handling that way...
I think I still prefer putting this logic in the HTTP client instead of having a separate error because all this handling is anyways contained in `RESTSessionCatalog` and abstracted away from users anyways.
--
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: Reference IRC to return 304-NotModified [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2425145754
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -360,6 +360,13 @@ protected <T extends RESTResponse> T execute(
}
}
+ private <T extends RESTResponse> boolean emptyBody(
+ CloseableHttpResponse response, Class<T> responseType) {
+ return response.getCode() == HttpStatus.SC_NO_CONTENT
+ || response.getCode() == HttpStatus.SC_NOT_MODIFIED
Review Comment:
Do we need the extra code check here? Wouldn't the `isSuccessful` already cover this?
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -433,8 +285,19 @@ public <T extends RESTResponse> T handleRequest(
tableIdentFromPathVars(vars),
snapshotModeFromQueryParams(httpRequest.queryParameters()));
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG, ETagProvider.of(response.metadataLocation())));
+ Optional<HTTPHeaders.HTTPHeader> ifNoneMatchHeader =
+ httpRequest.headers().entries().stream()
+ .filter(e -> e.name().equals(HttpHeaders.IF_NONE_MATCH))
+ .findFirst();
Review Comment:
I think there's a getFirstHeader on httpRequest, so I think we could simplify this a bit.
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -433,8 +285,19 @@ public <T extends RESTResponse> T handleRequest(
tableIdentFromPathVars(vars),
snapshotModeFromQueryParams(httpRequest.queryParameters()));
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG, ETagProvider.of(response.metadataLocation())));
+ Optional<HTTPHeaders.HTTPHeader> ifNoneMatchHeader =
+ httpRequest.headers().entries().stream()
+ .filter(e -> e.name().equals(HttpHeaders.IF_NONE_MATCH))
+ .findFirst();
+
+ String eTag = ETagProvider.of(response.metadataLocation());
+ Preconditions.checkNotNull(eTag, "ETag is null");
Review Comment:
I don't think this checkNotNull is neccessary.
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -114,147 +107,6 @@ public RESTCatalogAdapter(Catalog catalog) {
this.asViewCatalog = catalog instanceof ViewCatalog ? (ViewCatalog) catalog : null;
}
- enum Route {
Review Comment:
Could we publish a separate PR for this refactoring? I think we could get that in first. It'll make this change really just focused on what's necessary for supporting not modified
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2337040552
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
One more thought: This PR was meant to focus on the server side implementation of sending 304. What we are discussing now is more about how the client should handle such a response and I was going to take care of that in a follow-up PR. The overlap is that the TableErrorHandler is used both in `RESTCatalogAdapter.handleRequest()` to give error from the server, and also on the `HTTPClient.execute()` side on the client to throw exceptions based on the received responses.
I admit my client-side follow-up PoC catches NotModifiedException on the `RESTSessionCatalog.loadTable()` side, but I can adjust that to get null if the table is not changed. But still we'd need this `NotModifiedException` for the server side if I'm not mistaken.
--
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: Reference IRC to return 304-NotModified [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2426087084
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -360,6 +360,13 @@ protected <T extends RESTResponse> T execute(
}
}
+ private <T extends RESTResponse> boolean emptyBody(
+ CloseableHttpResponse response, Class<T> responseType) {
+ return response.getCode() == HttpStatus.SC_NO_CONTENT
+ || response.getCode() == HttpStatus.SC_NOT_MODIFIED
Review Comment:
This one is required here because this one triggers returning early with. Without this we'd run into an "Invalid (null) response body" RESTException.
However, the additional check into `isSuccessful` is not necessary required (same would be tru for SC_NO_CONTENT too), I think to be future proof it semantically makes sense for an `isSuccessful` function to return true for 304 too even though the current code returns before calling that function. WDYT?
Additionally, when I experimented around this area I noticed that the current test injects RESTCatalogAdapter directly into RESTSessionCatalog avoiding this code path through HTTPClient. Let me rethink the test a little bit so that we could go through the whole path.
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -433,8 +285,19 @@ public <T extends RESTResponse> T handleRequest(
tableIdentFromPathVars(vars),
snapshotModeFromQueryParams(httpRequest.queryParameters()));
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG, ETagProvider.of(response.metadataLocation())));
+ Optional<HTTPHeaders.HTTPHeader> ifNoneMatchHeader =
+ httpRequest.headers().entries().stream()
+ .filter(e -> e.name().equals(HttpHeaders.IF_NONE_MATCH))
+ .findFirst();
Review Comment:
Some of the classes from external hc.http source has such a function, however, this is the internal, Iceberg specific `HTTPRequest` class that doesn't. I can add such a function there, let me know if it makes sense (this or separate PR)
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -114,147 +107,6 @@ public RESTCatalogAdapter(Catalog catalog) {
this.asViewCatalog = catalog instanceof ViewCatalog ? (ViewCatalog) catalog : null;
}
- enum Route {
Review Comment:
Sure. Here it is: https://github.com/apache/iceberg/pull/14313
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -433,8 +285,19 @@ public <T extends RESTResponse> T handleRequest(
tableIdentFromPathVars(vars),
snapshotModeFromQueryParams(httpRequest.queryParameters()));
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG, ETagProvider.of(response.metadataLocation())));
+ Optional<HTTPHeaders.HTTPHeader> ifNoneMatchHeader =
+ httpRequest.headers().entries().stream()
+ .filter(e -> e.name().equals(HttpHeaders.IF_NONE_MATCH))
+ .findFirst();
+
+ String eTag = ETagProvider.of(response.metadataLocation());
+ Preconditions.checkNotNull(eTag, "ETag is null");
Review Comment:
Indeed, removed
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2335196208
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
OK I guess the argument against doing it in the http client is that if we just return null, it's a bit weird for clients to now have to worry about if (loadTableResponse != null) if it's a 304 vs getting an explicit exception and handling that way...
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2340178014
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -425,10 +429,23 @@ public <T extends RESTResponse> T handleRequest(
case LOAD_TABLE:
{
- LoadTableResponse resp = CatalogHandlers.loadTable(catalog, tableIdentFromPathVars(vars));
+ TableIdentifier ident = tableIdentFromPathVars(vars);
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG, ETagProvider.of(resp.metadataLocation())));
+ LoadTableResponse resp = CatalogHandlers.loadTable(catalog, ident);
Review Comment:
If I rename this then it won't be inline with the naming in other cases. These were added with my previous [PR](https://github.com/apache/iceberg/pull/12194).
So my question is: should I rename all the occurrences (then it would be kind of a noise to this PR), or should I rename this one (it would be inconsistent with others), or should I leave it as it is ?:)
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2341028141
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -425,10 +429,23 @@ public <T extends RESTResponse> T handleRequest(
case LOAD_TABLE:
{
- LoadTableResponse resp = CatalogHandlers.loadTable(catalog, tableIdentFromPathVars(vars));
+ TableIdentifier ident = tableIdentFromPathVars(vars);
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG, ETagProvider.of(resp.metadataLocation())));
+ LoadTableResponse resp = CatalogHandlers.loadTable(catalog, ident);
Review Comment:
I see, I think in this case I'd prefer to rename for just this one and cleanup the naming for the other occurrences in a separate PR since typically in the project we try not to use shortcut variable names. More of a preference, not a strict stance.
--
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: Reference IRC to return 304-NotModified [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#issuecomment-3452060615
Thanks for the reviews @amogh-jahagirdar @pvary !
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2358518093
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
@amogh-jahagirdar: How do we look at the RESTCatalogAdapter?
1. Is this just a reference implementation which show how should things work, and which could be used for testing the REST Client, or
2. Is it supposed to be a production ready code?
I'm asking, because for the 1st we need the simplest, most readable code. In this case I would prefer the `NotModifiedException` which clearly describes what is happening. If we aim for production code, I would prefer to avoid handling typical cases with exceptions. In this case, I would probably prefer the "null" check based solution.
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2379711095
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
Thanks for getting back with your thoughts, @amogh-jahagirdar !
I totally get your point that exceptions shouldn't be the way to control flow for non-error use cases. I did some digging around the area a bit and explored a couple of options. In summary what I think what makes it difficult for expressing non-200 codes from `RESTCatalogAdapter` is how we use it in our tests. There are 2 setups:
1) RESTSessionCatalog -> HttpClient -> RESTCatalogServlet -> RESTCatalogAdapter
2) RESTSessionCatalog -> RESTCatalogAdapter (basically mocking the server)
For scenario 1) we have more flexibility because we could abstract whatever we return from `RESTCatalogAdapter` either in `RESTCatalogServlet` by converting it to HTTP status codes, or even in `HttpClient`. Starting here I [created some PoC](https://github.com/gaborkaszab/iceberg/commit/a1ca9020506cc119863f077612c7bdccc3610c84#diff-7b4ef726f1b6b2d636070e7d50b9acce6e9f95ae1af9283850d1f80a34c7ef08) where I return a new `LoadTableResponseWithStatusCode` from `RESTCatalogAdapter` and then this could be translated into a 304 on the servlet level without a response body. In turn in `HttpClient` I could translate such responses into a null response.
However, the difficulty is scenario 2) where `RESTCatalogAdapter` is directly wired into `RESTSessionCatalog` as a client, because then whatever is returned from the adapter should be understood by the session catalog, including this new `...WithStatusCode` response. This isn't something I'd follow, because it would expose too much details to the session catalog that shouldn't be there.
So long story short, I think we can't do anything fancy here because of the current design. I see 2 options:
1) The current one with exceptions, but then these exceptions have to be caught within `RESTSessionCatalog` too that could be somewhat misleading to the readers because it's a success-case driven by exception.
2) Return null from `RESTCatalogAdapter.handleRequest()`. With this, on `RESTSessionCatalog.loadTable()` side we have to make null checks and associate getting nulls meaning the table is unchanged. I don't think this is very intuitive.
Btw, do you know the purpose of scenario 2) other than having simple tests mocking the communication with the server? I think it adds extra complexity for the reference IRC implementation a lot, and reduces the amount of complexity we have, e.g. with status code now. Would it make sense to get rid of such usage from the 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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2335142176
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -425,10 +429,23 @@ public <T extends RESTResponse> T handleRequest(
case LOAD_TABLE:
{
- LoadTableResponse resp = CatalogHandlers.loadTable(catalog, tableIdentFromPathVars(vars));
+ TableIdentifier ident = tableIdentFromPathVars(vars);
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG, ETagProvider.of(resp.metadataLocation())));
+ LoadTableResponse resp = CatalogHandlers.loadTable(catalog, ident);
Review Comment:
Nit: can we use the full word `response` to better fit the style of the project
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
Have we considered putting the handling of 304 in the actual HTTP client https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java#L326 similar to 204 handling? I think the argument there is we should just be able to return null for the table response and then we don't really need to treat this like an "error" case.
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
I'm not sure I agree with clients handling the 304 from the server as an error and throwing . I see how it's convenient though in the current implementation since then it's just a try/catch and also in this case there's no real body to work with in the response. In my mind, it's fundamentally not an error so modeling it as an exception doesn't seem right.
I'll need to think more if there's a better alternative though
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2343303670
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -425,10 +429,23 @@ public <T extends RESTResponse> T handleRequest(
case LOAD_TABLE:
{
- LoadTableResponse resp = CatalogHandlers.loadTable(catalog, tableIdentFromPathVars(vars));
+ TableIdentifier ident = tableIdentFromPathVars(vars);
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG, ETagProvider.of(resp.metadataLocation())));
+ LoadTableResponse resp = CatalogHandlers.loadTable(catalog, ident);
Review Comment:
merged the other PR and did a rebase here. This is resolved now
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2358712730
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
Thank you for taking a look, @pvary !
The thing with changing response types (and parameters) is that it is an API breaking change. Not directly for `RESTCatalogAdapter.handleRequest()` but for the calling `RESTCatalogAdapter.execute()` that invokes this, because it is an override of `HTTPRequest.execute()`.
So with the current design of the reference IRC, I think we can either follow the exception driven approach, or implement some (not that foolproof) checks in `RESTCatalogServlet` like "if the responseBody is null and the load table endpoint was called, then return 304". The exception-driven approach seems to be the cleanest on the reference IRC side.
cc @amogh-jahagirdar @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: Reference IRC to return 304-NotModified [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#issuecomment-3421164367
The changes I made recently on this PR:
- Rebased with main to have the Route class refactor
- Noticed, the current test mocks the adapter in RESTSessionCatalog and as a result the query doesn't go through HTTPClient and RESTCatalogServlet. Made some adjustments on the tests to be able to mock the adapter within the servlet instead of the one within RESTSessionCatalog to not test the whole chain.
@amogh-jahagirdar Do you think you have some capacity to take a look?
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2377137224
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
Sorry for the delay, @gaborkaszab @pvary .
So @pvary, I do think it's like point 1 where it's a reference implementation of showing how things should work but i'm also aiming for "principle of least surprise" here. Also I just am a little bit against exposing concepts like exceptions for representing cases where it's not really exceptional. At the end of the day the 304 is really the server telling the client "hey use your cache".
Separating out the client side and server side:
1.) On the client side, after looking a bit further, clients like the Github cli and other Http client frameworks do essentially handle null response bodies for 304 and it's expected that the response body is null for a 304.
2.) On the server side, it sounds like the reason for adding the exception was that our current implementation of `RESTCatalogAdapter` doesn't have a way of expressing non-200 codes without an exception? I feel like that's the limitation to address in `RESTCatalogAdapter`.
In the end I'm mostly +0 if we want to do the exception mostly because it just feels like exposing an unnecessary concept to work around the current reference server handling, and that can lead to people misusing that exception since it's in the public API. It'd be ideal if we can at least show how complicated the other solution is for being able to return status codes other than 200 without an exception.
--
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: Reference IRC to return 304-NotModified [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #14035:
URL: https://github.com/apache/iceberg/pull/14035
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2335196208
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
OK I guess the argument against doing it in the http client is that if we just return null, it's a bit weird for clients to now have to worry about if (loadTableResponse != null) if it's a 304 vs getting an explicit exception and handling that way...
I think I still prefer putting this logic in the HTTP client instead of treating it as an error because all this handling is anyways contained in `RESTSessionCatalog` and abstracted away from users anyways.
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2337148163
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
I've checked the server-side (i.e. RESTCatalogAdapter) of the code a bit more deeply, and apparently the result code it sets in the response is either 200 or the one associated with an Exception. It can't even send 204. So based on this, we still need the new exception class to send a 304 error code from the reference IRC, or we'd need a bit of a restructuring of the handling of the response codes in `RESTCatalogAdapter` and `RESTCatalogServlet`
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2337040552
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
One more thought: This PR was meant to focus on the server side implementation of sending 304. What we are discussing now is more about how the client should handle such a response and I was going to take care of that in a follow-up PR. The overlap is that the TableErrorHandler is used both in `RESTCatalogAdapter.handleRequest()` to give error from the server, and also on the `HTTPClient.execute()` side on the client to throw exceptions based on the received responses.
I admit my client-side follow-up PoC catches NotModifiedException on the `RESTSessionCatalog.loadTable()` side, but I can adjust that to get null if the table is not changed. But still we'd need this `NotModifiedException` for the server side if I'm not mistaken.
Update: Well, taking a second look, maybe I can construct a response without the exception too. Is your point that we can implement this without the exception on the server-side too?
--
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: Reference IRC to return 304-NotModified [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#issuecomment-3390653951
Hey @amogh-jahagirdar ,
Would you mind taking a look at this approach?
--
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: Reference IRC to return 304-NotModified [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#issuecomment-3390741034
Hey sorry for the delay @gaborkaszab yes this is on my list to review today!
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2341240240
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -425,10 +429,23 @@ public <T extends RESTResponse> T handleRequest(
case LOAD_TABLE:
{
- LoadTableResponse resp = CatalogHandlers.loadTable(catalog, tableIdentFromPathVars(vars));
+ TableIdentifier ident = tableIdentFromPathVars(vars);
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG, ETagProvider.of(resp.metadataLocation())));
+ LoadTableResponse resp = CatalogHandlers.loadTable(catalog, ident);
Review Comment:
I created a PR to rename all of these together: https://github.com/apache/iceberg/pull/14051
I prefer not to do it partially here as these variables weren't introduced in this PR anyway.
--
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] API, Core: Reference IRC to throw 304-NotModifiedException [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#discussion_r2335769947
##########
api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/** Exception to indicate that resource is unchanged. */
+public class NotModifiedException extends RESTException {
Review Comment:
This was exactly my thought process, I just came to a different conclusion :)
My argument here is that we associate LoadTableResponse == null with a meaning (304-NotModified) that works now but:
- it's not that intuitive when reading the RESTSessionCatalog code
- Not entirely future proof, because if later some other mechanism also returns null for non-304 scenarios then it's hard to differentiate in RESTSessionCatalog why the result is null
- Having to catch NotModifiedException in RESTSessionCatalog.loadTable() is clear and intuitive, because we explicitly articulate with catching the exception that this is the case when the table is unchanged (and not comparing results to null)
WDYT?
--
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: Reference IRC to return 304-NotModified [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#issuecomment-3451908533
Thanks @gaborkaszab and @pvary for reviewing
--
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: Reference IRC to return 304-NotModified [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#issuecomment-3398142449
FYI, I did a rebase with master to have the Route refactor
--
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: Reference IRC to return 304-NotModified [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #14035:
URL: https://github.com/apache/iceberg/pull/14035#issuecomment-3370533074
Hi @amogh-jahagirdar
I've published the alternative approach within this PR. Would you mind taking a look?
--
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