Use ObjectMapper from SolrJacksonMapper in CachingJsonMessageBodyReader#4370
Use ObjectMapper from SolrJacksonMapper in CachingJsonMessageBodyReader#4370renatoh wants to merge 7 commits intoapache:mainfrom
Conversation
…ctMapper with default config
renatoh
changed the title
|
While this makes sense to me, I don't know the ramifications of this change.. I'm tagging @gerlowskija as well. I did trigger the tests to run! |
|
@dsmiley would be great to get your take on that change, thanks! |
There was a problem hiding this comment.
This isn't changelog worthy, at least not as-written. No user of Solr will care.
There was a problem hiding this comment.
This isn't changelog worthy, at least not as-written. No user of Solr will care.
I am a bit confused what does need a changelog and what not. Small changes without a associate Jira do not need a change log?
What you think of the fix itself, is this a fix or improvement in your opinion?
There was a problem hiding this comment.
Wether there's a JIRA or not doesn't matter, although it tends to be correlated as something not changelog worthy isn't JIRA-worthy either... although we sometimes create JIRAs to remember to do something which is quite alright.
The way I think of a writing a changelog entry is, I think, rather simple. Step back from the nitty gritty details of the PR and imagine telling a a stranger who uses Solr about what you did in a way that hopefully might interest them. The changelog speaks to the user -- it's a summary that many users read each release. How is this change making Solr better? If it's too much of a stretch to write something of interest then don't bother wasting both your time and more importantly all potential readers of it.
The "Other" section can be an exception to this as it's more open up to Solr hackers to record more significant changes that won't interest users. Changes significant enough that us Solr hackers (Solr plugin authors & Solr contributors) may want to remember that a notable thing happened in this Solr version, like a significant (not insignificant) refactoring.
And no-changelog doesn't mean a PR isn't appreciated!
What you think of the fix itself, is this a fix or improvement in your opinion?
All I could discern of interest is the support for comments. I'm unsure if this was possible before? Seems too minor to mention IMO.
There was a problem hiding this comment.
I assume you saw this text in the process of producing the changelog? I seek any feedback you may have to improve it in light of what I said above.
There was a problem hiding this comment.
No, I've not seen this text. I added a change log since I have be told to add one on previous PRs, which were also minor.
But good to know, I think the important point is the one about the audience, I was not aware the audience are not the committers, thanks for pointing it out.
|
I looked at the unit tests, and yes, if you revert the change, the unit test fails..... I am a bit surprised that this bug hasn't been seen before? Can you share a bit on how you stumbled across this? Is this something that can actually happen in a running Solr, or does it only happen in the way you set it up. Also, I have seen recently in another project that apparently ObjectMapper's are expensive to create and threadsafe so they SHOULD be reused? |
|
Lastly, I added |
Yes, they are thread-safe and should be reused, that is also stated in the java-doc: And indeed, the ObjectMapper has not been reused before, I have added a test to ensure it is reused now, and this test is failing with the MessageBodyReaders prior my change. I came across working on this ticket: |
|
Yeah, you may be discovering an issue in us not reusing the ObjectMapper? Do you think you need to dig a bit more and see if there is a larger change that should be made? Or is THIS that fix? Looking at the |
|
At the moment I do not understand the scope of the issue since the only endpoint I found invoking org.apache.solr.jersey.MessageBodyReaders.CachingJsonMessageBodyReader#getDelegate is the one to create a collection on /api/collections All other calls to /api/... do not seem to call it, at least the once I've tried. So why wouldn't we want to use CachingJsonMessageBodyReader consistently is the question I do not understand. |
3 participants
Looking into https://issues.apache.org/jira/browse/SOLR-18191 I released our V2API not using SolrJacksonMapper.getObjectMapper(). This feels wrong to me.