Re: [PR] Core: Reference IRC to return 204 [iceberg]
kevinjqliu commented on PR #14724: URL: https://github.com/apache/iceberg/pull/14724#issuecomment-3617664366 Thanks for the PR @gaborkaszab and thanks everyone 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
kevinjqliu merged PR #14724: URL: https://github.com/apache/iceberg/pull/14724 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
kevinjqliu commented on PR #14724: URL: https://github.com/apache/iceberg/pull/14724#issuecomment-3608003205 devlist discussion (i cant find the original thread for some reason), https://lists.apache.org/thread/876dhnf4mbsr2yfvj3kx6jf034zs432q if there are no objections, i'll merge this by EOW :) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
gaborkaszab commented on PR #14724: URL: https://github.com/apache/iceberg/pull/14724#issuecomment-3606274922 Thank you for taking a look @kevinjqliu @pvary @amogh-jahagirdar @nastra ! I fixed that nit and also sent a mail to the dev@ list. Let's see if there are anyone against. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
kevinjqliu commented on code in PR #14724:
URL: https://github.com/apache/iceberg/pull/14724#discussion_r2582517618
##
core/src/test/java/org/apache/iceberg/rest/RESTCatalogServlet.java:
##
@@ -133,6 +135,20 @@ protected void execute(ServletRequestContext context,
HttpServletResponse respon
}
}
+ private boolean canReturn204(Route route) {
Review Comment:
`shouldReturn204` makes sense to me.
and yes, good point. Route already has both
https://github.com/apache/iceberg/blob/da76b873fad9b87c76eed1e91185e7b9fcb9a7f8/core/src/test/java/org/apache/iceberg/rest/Route.java#L62
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
amogh-jahagirdar commented on code in PR #14724:
URL: https://github.com/apache/iceberg/pull/14724#discussion_r2582424154
##
core/src/test/java/org/apache/iceberg/rest/RESTCatalogServlet.java:
##
@@ -133,6 +135,20 @@ protected void execute(ServletRequestContext context,
HttpServletResponse respon
}
}
+ private boolean canReturn204(Route route) {
Review Comment:
The route enum already encapsulates the method as well so we should be OK
here. My nit here is I think this method should be called `shouldReturn204`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
kevinjqliu commented on PR #14724: URL: https://github.com/apache/iceberg/pull/14724#issuecomment-3603308376 > Although this change aligns the implementation with the specification, I believe it warrants a note to the dev list, as it introduces a significant behavioral change. I think we should send out a notification to the devlist just to inform folks about this behavior change for the `iceberg-rest-fixture` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
kevinjqliu commented on code in PR #14724:
URL: https://github.com/apache/iceberg/pull/14724#discussion_r2582267189
##
core/src/test/java/org/apache/iceberg/rest/RESTCatalogServlet.java:
##
@@ -133,6 +135,20 @@ protected void execute(ServletRequestContext context,
HttpServletResponse respon
}
}
+ private boolean canReturn204(Route route) {
Review Comment:
nit: should we gate this using both the route and method (GET/POST/HEAD)?
For example, in the `/v1/{prefix}/namespaces/{namespace}` route, only HEAD
supports 204
https://github.com/apache/iceberg/blob/29a144adca05344de2a8aab05e2b3385c4053c8f/open-api/rest-catalog-open-api.yaml#L383
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
pvary commented on PR #14724: URL: https://github.com/apache/iceberg/pull/14724#issuecomment-3601595263 I leave a few days for others to review. Maybe @nastra would be interested in this as he was working around this part of the code quite a bit. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
pvary commented on PR #14724: URL: https://github.com/apache/iceberg/pull/14724#issuecomment-3596209199 Although this change aligns the implementation with the specification, I believe it warrants a note to the dev list, as it introduces a significant behavioral change. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Core: Reference IRC to return 204 [iceberg]
gaborkaszab commented on PR #14724: URL: https://github.com/apache/iceberg/pull/14724#issuecomment-3595889493 I don't think there is any way to test status codes returned by the reference IRC. For that HTTPClient would require a couple of changes, like wrapping the function call performing the client-server exchange into a function so that we can capture the response msg from the server in tests by spying this function. I can take a look at this, but couldn't convince myself such a change is beneficial. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
