amogh-jahagirdar commented on code in PR #13004:
URL: https://github.com/apache/iceberg/pull/13004#discussion_r2121655388
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -329,7 +341,8 @@ protected <T extends RESTResponse> T execute(
}
try {
- return mapper.readValue(responseBody, responseType);
+ ObjectReader reader =
mapper.readerFor(responseType).with(injectableValues);
Review Comment:
Ok if I understand right, we'll be creating a new object reader per
request/response cycle and plumbing through any context that way. This is nice
since it would address any isolation concerns between requests and shouldn't
have any performance issues since it should be doing the same amount of work
that the existing `readValue` API was doing (that'd have to create some kind of
reader under the hood for the specific type being requested)....if that's the
case, that seems like a good direction. cc @danielcweeks @rdblue
(see my comment on the other
[PR](https://github.com/apache/iceberg/pull/13191#discussion_r2121617604) about
how I think we should be passing in our own context class and then creating the
implementation specific `injectableValues` just at this point).
##########
core/src/main/java/org/apache/iceberg/rest/RESTObjectMapper.java:
##########
@@ -26,7 +26,7 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.PropertyNamingStrategies;
-class RESTObjectMapper {
+public class RESTObjectMapper {
Review Comment:
Why did this need to be made public?
--
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]
