-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug fix @DruidSecondaryModule plus unit test #1245
Conversation
Hi @b-slim , thanks for the PR.
|
The injector creation workflow usually goes through cusotom Druid Guice handlings rather than making a Guice injector using Guice directly. These are setup during the CLI launching workflow. Did this come about during actually running a service? (the CLI command workflow should have injected stuff properly) or during a unit test? Guice injection in a unit test is a PITA right now. |
Bug fix @DruidSecondaryModule plus unit test
@drcrallen It is come when running any service. The reason i used the class name emitter because it did happen when i did used my own custom emitter module. |
@drcrallen If you look at how Guice is setup, the unit test is replicating exactly how Guice works inside of Druid. This is a real problem in the current code as evidenced by the unit test that now passes. |
@cheddar Unless you're actually calling the methods in I don't fully understand why "make objectMapper eager to ensure jackson gets setup with guice injection for JsonConfigurator" is no longer a relevant concern. But the PR is in so there's not much use in dwelling on it. |
@drcrallen I trust @cheddar on this one, since he initially wrote the Guice injection bootstrapping and it hasn't changed much since. I assume this has been tested standalone and we'll have to make sure it doesn't break any of the examples or extensions being able to register jackson types. @b-slim @cheddar since you are working on making things more pluggable, it would be really cool if you could write some documentation on pluggable things, e.g. showing how you can build your own emitter module. Maybe we can add a section for that to the documentation revamp. |
@xvrl That sounds a good idea, will try to write some doc about it. |
@b-slim Just an FYI, if you've been following the Druid proposals, I am currently making gigantic changes to docs right now. It may be a good idea to wait until my changes are in before making any doc changes on your side. |
This PR has a fix and unit test to catch the bug.
The bug will raise when a module use the
JsonConfigurator
while the Guice built in.asEagerSingleton()
start forObjectMapper
did not initialize it yet.