[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[ https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17721368#comment-17721368 ] ASF GitHub Bot commented on DRILL-8430: --- 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. > add factory method for creating Jackson ObjectMappers > - > > Key: DRILL-8430 > URL: https://issues.apache.org/jira/browse/DRILL-8430 > Project: Apache Drill > Issue Type: Task > Components: Server >Reporter: PJ Fanning >Priority: Major > > See https://issues.apache.org/jira/browse/DRILL-8415 > It's useful to keep any customisation of the ObjectMapper creation in 1 place -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[ https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17721309#comment-17721309 ] ASF GitHub Bot commented on DRILL-8430: --- 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. > add factory method for creating Jackson ObjectMappers > - > > Key: DRILL-8430 > URL: https://issues.apache.org/jira/browse/DRILL-8430 > Project: Apache Drill > Issue Type: Task > Components: Server >Reporter: PJ Fanning >Priority: Major > > See https://issues.apache.org/jira/browse/DRILL-8415 > It's useful to keep any customisation of the ObjectMapper creation in 1 place -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[
https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17721262#comment-17721262
]
ASF GitHub Bot commented on DRILL-8430:
---
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.
> add factory method for creating Jackson ObjectMappers
> -
>
> Key: DRILL-8430
> URL: https://issues.apache.org/jira/browse/DRILL-8430
> Project: Apache Drill
> Issue Type: Task
> Components: Server
>Reporter: PJ Fanning
>Priority: Major
>
> See https://issues.apache.org/jira/browse/DRILL-8415
> It's useful to keep any customisation of the ObjectMapper creation in 1 place
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[
https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17721260#comment-17721260
]
ASF GitHub Bot commented on DRILL-8430:
---
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.
> add factory method for creating Jackson ObjectMappers
> -
>
> Key: DRILL-8430
> URL: https://issues.apache.org/jira/browse/DRILL-8430
> Project: Apache Drill
> Issue Type: Task
> Components: Server
>Reporter: PJ Fanning
>Priority: Major
>
> See https://issues.apache.org/jira/browse/DRILL-8415
> It's useful to keep any customisation of the ObjectMapper creation in 1 place
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[
https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17721257#comment-17721257
]
ASF GitHub Bot commented on DRILL-8430:
---
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.
> add factory method for creating Jackson ObjectMappers
> -
>
> Key: DRILL-8430
> URL: https://issues.apache.org/jira/browse/DRILL-8430
> Project: Apache Drill
> Issue Type: Task
> Components: Server
>Reporter: PJ Fanning
>Priority: Major
>
> See https://issues.apache.org/jira/browse/DRILL-8415
> It's useful to keep any customisation of the ObjectMapper creation in 1 place
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[
https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17721253#comment-17721253
]
ASF GitHub Bot commented on DRILL-8430:
---
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.
> add factory method for creating Jackson ObjectMappers
> -
>
> Key: DRILL-8430
> URL: https://issues.apache.org/jira/browse/DRILL-8430
> Project: Apache Drill
> Issue Type: Task
> Components: Server
>Reporter: PJ Fanning
>Priority: Major
>
> See https://issues.apache.org/jira/browse/DRILL-8415
> It's useful to keep any customisation of the ObjectMapper creation in 1 place
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[
https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17721065#comment-17721065
]
ASF GitHub Bot commented on DRILL-8430:
---
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.
> add factory method for creating Jackson ObjectMappers
> -
>
> Key: DRILL-8430
> URL: https://issues.apache.org/jira/browse/DRILL-8430
> Project: Apache Drill
> Issue Type: Task
> Components: Server
>Reporter: PJ Fanning
>Priority: Major
>
> See https://issues.apache.org/jira/browse/DRILL-8415
> It's useful to keep any customisation of the ObjectMapper creation in 1 place
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[ https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17720913#comment-17720913 ] ASF GitHub Bot commented on DRILL-8430: --- 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? > add factory method for creating Jackson ObjectMappers > - > > Key: DRILL-8430 > URL: https://issues.apache.org/jira/browse/DRILL-8430 > Project: Apache Drill > Issue Type: Task > Components: Server >Reporter: PJ Fanning >Priority: Major > > See https://issues.apache.org/jira/browse/DRILL-8415 > It's useful to keep any customisation of the ObjectMapper creation in 1 place -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[ https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17720908#comment-17720908 ] ASF GitHub Bot commented on DRILL-8430: --- 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? > add factory method for creating Jackson ObjectMappers > - > > Key: DRILL-8430 > URL: https://issues.apache.org/jira/browse/DRILL-8430 > Project: Apache Drill > Issue Type: Task > Components: Server >Reporter: PJ Fanning >Priority: Major > > See https://issues.apache.org/jira/browse/DRILL-8415 > It's useful to keep any customisation of the ObjectMapper creation in 1 place -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[ https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17720907#comment-17720907 ] ASF GitHub Bot commented on DRILL-8430: --- 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? > add factory method for creating Jackson ObjectMappers > - > > Key: DRILL-8430 > URL: https://issues.apache.org/jira/browse/DRILL-8430 > Project: Apache Drill > Issue Type: Task > Components: Server >Reporter: PJ Fanning >Priority: Major > > See https://issues.apache.org/jira/browse/DRILL-8415 > It's useful to keep any customisation of the ObjectMapper creation in 1 place -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[ https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17720906#comment-17720906 ] ASF GitHub Bot commented on DRILL-8430: --- 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? > add factory method for creating Jackson ObjectMappers > - > > Key: DRILL-8430 > URL: https://issues.apache.org/jira/browse/DRILL-8430 > Project: Apache Drill > Issue Type: Task > Components: Server >Reporter: PJ Fanning >Priority: Major > > See https://issues.apache.org/jira/browse/DRILL-8415 > It's useful to keep any customisation of the ObjectMapper creation in 1 place -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[ https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17720905#comment-17720905 ] ASF GitHub Bot commented on DRILL-8430: --- 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? > add factory method for creating Jackson ObjectMappers > - > > Key: DRILL-8430 > URL: https://issues.apache.org/jira/browse/DRILL-8430 > Project: Apache Drill > Issue Type: Task > Components: Server >Reporter: PJ Fanning >Priority: Major > > See https://issues.apache.org/jira/browse/DRILL-8415 > It's useful to keep any customisation of the ObjectMapper creation in 1 place -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (DRILL-8430) add factory method for creating Jackson ObjectMappers
[ https://issues.apache.org/jira/browse/DRILL-8430?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17720892#comment-17720892 ] ASF GitHub Bot commented on DRILL-8430: --- 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.) > add factory method for creating Jackson ObjectMappers > - > > Key: DRILL-8430 > URL: https://issues.apache.org/jira/browse/DRILL-8430 > Project: Apache Drill > Issue Type: Task > Components: Server >Reporter: PJ Fanning >Priority: Major > > See https://issues.apache.org/jira/browse/DRILL-8415 > It's useful to keep any customisation of the ObjectMapper creation in 1 place -- This message was sent by Atlassian Jira (v8.20.10#820010)
