amogh-jahagirdar commented on code in PR #6835:
URL: https://github.com/apache/iceberg/pull/6835#discussion_r1107874421
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3ObjectMapper.java:
##########
@@ -48,13 +48,16 @@ public class S3ObjectMapper {
private S3ObjectMapper() {}
- static ObjectMapper mapper() {
+ public static ObjectMapper mapper() {
if (!isInitialized) {
synchronized (S3ObjectMapper.class) {
Review Comment:
Something I missed earlier but it's minor, generally for these kinds of
initializations I think it's more clear to have an
AtomicReference<ObjectMapper>() and use the `compareAndSet` method if the
instance is null.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3ObjectMapper.java:
##########
@@ -48,13 +48,16 @@ public class S3ObjectMapper {
private S3ObjectMapper() {}
- static ObjectMapper mapper() {
+ public static ObjectMapper mapper() {
Review Comment:
Does the underlying object mapper need to be public? It feels like that
shouldn't be if this `S3ObjectMapper` class is supposed to expose mapper
operations
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3ObjectMapper.java:
##########
@@ -48,13 +48,16 @@ public class S3ObjectMapper {
private S3ObjectMapper() {}
- static ObjectMapper mapper() {
+ public static ObjectMapper mapper() {
if (!isInitialized) {
synchronized (S3ObjectMapper.class) {
if (!isInitialized) {
MAPPER.setVisibility(PropertyAccessor.FIELD,
JsonAutoDetect.Visibility.ANY);
MAPPER.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES,
false);
-
MAPPER.setPropertyNamingStrategy(PropertyNamingStrategies.KebabCaseStrategy.INSTANCE);
+ // even though using new PropertyNamingStrategy.KebabCaseStrategy()
is deprecated
+ // and PropertyNamingStrategies.KebabCaseStrategy.INSTANCE
(introduced in jackson 1.14) is
+ // recommended, we can't use it because Spark still relies on
jackson 1.13.x stuff
Review Comment:
Do we mean Jackson `2.13` and `2.14` instead of `1.x` here? I think this is
okay but ideally what we do is we upgrade Iceberg Spark jackson itself then we
can use the non-deprecated version.
--
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]
