You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by "pjfanning (via GitHub)" <gi...@apache.org> on 2023/05/09 10:25:44 UTC
[PR] [DRILL-8430] create JacksonUtils (drill)
pjfanning opened a new pull request, #2800:
URL: https://github.com/apache/drill/pull/2800
## Description
Shared utility methods for Jackson
## Documentation
(Please describe user-visible changes similar to what should appear in the Drill documentation.)
## Testing
(Please describe how this PR has been tested.)
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre commented on PR #2800:
URL: https://github.com/apache/drill/pull/2800#issuecomment-1540031437
@pjfanning There is an issue with Calcite 1.35-SNAPSHOT that is causing `TestTimestampAddDiffFunctions.testTimestampAddDiffLiteralTypeInference` to fail. This has nothing to do with this PR. Would you mind putting an `Ignore` with a note on that unit test until we fix it so that we can test this PR?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre commented on PR #2800:
URL: https://github.com/apache/drill/pull/2800#issuecomment-1540012488
@pjfanning There is an issue with Calcite 1.35-SNAPSHOT that is causing ` TestTimestampAddDiffFunctions.testTimestampAddDiffLiteralTypeInference` to fail. This has nothing to do with this PR. Would you mind putting an `@Ignore` with a note on that unit test until we fix it so that we can test this PR?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre commented on PR #2800:
URL: https://github.com/apache/drill/pull/2800#issuecomment-1542048115
@pjfanning Sorry for the repeated comments yesterday. Github was giving me an error message and it didn't look like the comment had actually posted.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #2800:
URL: https://github.com/apache/drill/pull/2800#discussion_r1189058399
##########
common/src/test/java/org/apache/drill/test/DrillTest.java:
##########
@@ -37,11 +38,10 @@
public class DrillTest extends BaseTest {
- protected static final ObjectMapper objectMapper;
+ private static final ObjectMapper objectMapper = JacksonUtils.createObjectMapper();
Review Comment:
Some tests were using this mapper and modifying it - which seems like a dangerous thing to do because other tests will be affected by the modified mapper. I've changed the tests that used this mapper to create their own separate mappers.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "jnturton (via GitHub)" <gi...@apache.org>.
jnturton commented on code in PR #2800:
URL: https://github.com/apache/drill/pull/2800#discussion_r1189578197
##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/http/oauth/OAuthUtils.java:
##########
@@ -36,6 +37,7 @@
public class OAuthUtils {
private static final Logger logger = LoggerFactory.getLogger(OAuthUtils.class);
+ private static final ObjectMapper MAPPER = JacksonUtils.createObjectMapper();
Review Comment:
Hmm, reading [on little](https://stackoverflow.com/a/36162525/1153953) reveals that lock contention in a shared ObjectMapper degrades performance for multithreaded applications . Now I'm in two minds.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/http/oauth/OAuthUtils.java:
##########
@@ -36,6 +37,7 @@
public class OAuthUtils {
private static final Logger logger = LoggerFactory.getLogger(OAuthUtils.class);
+ private static final ObjectMapper MAPPER = JacksonUtils.createObjectMapper();
Review Comment:
Hmm, reading [on a little](https://stackoverflow.com/a/36162525/1153953) reveals that lock contention in a shared ObjectMapper degrades performance for multithreaded applications . Now I'm in two minds.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre commented on PR #2800:
URL: https://github.com/apache/drill/pull/2800#issuecomment-1547096494
@jnturton Can we merge 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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre commented on PR #2800:
URL: https://github.com/apache/drill/pull/2800#issuecomment-1540012829
@pjfanning There is an issue with Calcite 1.35-SNAPSHOT that is causing ` TestTimestampAddDiffFunctions.testTimestampAddDiffLiteralTypeInference` to fail. This has nothing to do with this PR. Would you mind putting an `Ignore` with a note on that unit test until we fix it so that we can test this PR?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "jnturton (via GitHub)" <gi...@apache.org>.
jnturton commented on code in PR #2800:
URL: https://github.com/apache/drill/pull/2800#discussion_r1189557009
##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/http/oauth/OAuthUtils.java:
##########
@@ -36,6 +37,7 @@
public class OAuthUtils {
private static final Logger logger = LoggerFactory.getLogger(OAuthUtils.class);
+ private static final ObjectMapper MAPPER = JacksonUtils.createObjectMapper();
Review Comment:
We're converting method scope ObjectMappers to static class members which is efficient in terms of rework but will add a bit to Drill's fixed heap requirement since they can never be collected. It looks like ObjectMappers are thread safe so, for the cases where the caller does not need to do mapper customisation, could we get even better reuse from a new singleton `JacksonUtils.DEFAULT_MAPPER`?
##########
common/src/test/java/org/apache/drill/test/DrillTest.java:
##########
@@ -37,11 +38,10 @@
public class DrillTest extends BaseTest {
- protected static final ObjectMapper objectMapper;
+ private static final ObjectMapper objectMapper = JacksonUtils.createObjectMapper();
Review Comment:
Nice catch, thank you.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre commented on PR #2800:
URL: https://github.com/apache/drill/pull/2800#issuecomment-1540013301
@pjfanning There is an issue with Calcite 1.35-SNAPSHOT that is causing ` TestTimestampAddDiffFunctions.testTimestampAddDiffLiteralTypeInference` to fail. This has nothing to do with this PR. Would you mind putting an `Ignore` with a note on that unit test until we fix it so that we can test this PR?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre commented on PR #2800:
URL: https://github.com/apache/drill/pull/2800#issuecomment-1540012740
@pjfanning There is an issue with Calcite 1.35-SNAPSHOT that is causing ` TestTimestampAddDiffFunctions.testTimestampAddDiffLiteralTypeInference` to fail. This has nothing to do with this PR. Would you mind putting an `@Ignore` with a note on that unit test until we fix it so that we can test this PR?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "jnturton (via GitHub)" <gi...@apache.org>.
jnturton merged PR #2800:
URL: https://github.com/apache/drill/pull/2800
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #2800:
URL: https://github.com/apache/drill/pull/2800#issuecomment-1542255539
@cgivre no worries - I had timeout and other issues with GitHub around that time too. Some sort of general GitHub health issues, I guess.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "jnturton (via GitHub)" <gi...@apache.org>.
jnturton commented on code in PR #2800:
URL: https://github.com/apache/drill/pull/2800#discussion_r1189562746
##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/http/oauth/OAuthUtils.java:
##########
@@ -36,6 +37,7 @@
public class OAuthUtils {
private static final Logger logger = LoggerFactory.getLogger(OAuthUtils.class);
+ private static final ObjectMapper MAPPER = JacksonUtils.createObjectMapper();
Review Comment:
Follow up: [this post](https://stackoverflow.com/a/3909846/1153953) suggests that global ObjectReader and ObjectWriter objects might be a better choice than a global ObjectMapper.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8430: Create JacksonUtils (drill)
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #2800:
URL: https://github.com/apache/drill/pull/2800#discussion_r1189568415
##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/http/oauth/OAuthUtils.java:
##########
@@ -36,6 +37,7 @@
public class OAuthUtils {
private static final Logger logger = LoggerFactory.getLogger(OAuthUtils.class);
+ private static final ObjectMapper MAPPER = JacksonUtils.createObjectMapper();
Review Comment:
the problem with global object mappers, writers and readers is that if they are public, then someone can modify their config - exactly the issue I found in our test code
I opened https://issues.apache.org/jira/browse/DRILL-8431 to look at wrapping the Jackson classes to create immutable instances that can be more safely shared. So far, that looks like a lot of work and the benefits may not be worth it.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org