Skip to content

fix circular reference in moving average query#8192

Merged
clintropolis merged 1 commit intoapache:masterfrom
ccl0326:ccl0326-patch-1
Jul 30, 2019
Merged

fix circular reference in moving average query#8192
clintropolis merged 1 commit intoapache:masterfrom
ccl0326:ccl0326-patch-1

Conversation

@yurmix
Copy link
Copy Markdown
Contributor

@yurmix yurmix commented Jul 29, 2019

Fixes #7999.
Contributed by @ccl0326.

@yurmix
Copy link
Copy Markdown
Contributor Author

yurmix commented Jul 29, 2019

@ccl0326 thank you for your contribution! I have verified that this fixes the issue.
I have taken the liberty to create a pull request so it will be ready for 0.15.1, but if you prefer, I can close this one and you can create a separate pull request.

@clintropolis clintropolis added this to the 0.15.1 milestone Jul 29, 2019
@clintropolis
Copy link
Copy Markdown
Member

This appears to partially fix the issue in my testing, at least in terms of the broker being unable to start.

However, there are some additional difficulties to use this extension. The middle-manager is unable to load this extension, it explodes on startup with a similar:

Exception in thread "main" java.lang.RuntimeException: com.google.inject.CreationException: Unable to create injector, see the following errors:

1) No implementation for org.apache.druid.query.QuerySegmentWalker was bound.
  while locating com.google.inject.Provider<org.apache.druid.query.QuerySegmentWalker>
    for the 1st parameter of org.apache.druid.query.movingaverage.MovingAverageQueryToolChest.<init>(MovingAverageQueryToolChest.java:62)
  at org.apache.druid.query.movingaverage.MovingAverageQueryModule.configure(MovingAverageQueryModule.java:48) (via modules: com.google.inject.util.Modules$OverrideModule -> org.apache.druid.query.movingaverage.MovingAverageQueryModule)

2) No implementation for org.apache.druid.server.log.RequestLogger was bound.
  while locating org.apache.druid.server.log.RequestLogger
    for the 2nd parameter of org.apache.druid.query.movingaverage.MovingAverageQueryToolChest.<init>(MovingAverageQueryToolChest.java:62)
  at org.apache.druid.query.movingaverage.MovingAverageQueryModule.configure(MovingAverageQueryModule.java:48) (via modules: com.google.inject.util.Modules$OverrideModule -> org.apache.druid.query.movingaverage.MovingAverageQueryModule)

so druid.indexer.fork.property.druid.extensions.loadList must be set to allow the peons to load a different set of extensions than the middle-manager, which isn't so big of a deal imo. The peon tasks for kafka indexing service did appear to startup and load the extension correctly when I did this.

More problematic, is the router, which is also unable to load this extension:

Exception in thread "main" java.lang.RuntimeException: com.google.inject.CreationException: Unable to create injector, see the following errors:

1) No implementation for org.apache.druid.query.QuerySegmentWalker was bound.
  while locating com.google.inject.Provider<org.apache.druid.query.QuerySegmentWalker>
    for the 1st parameter of org.apache.druid.query.movingaverage.MovingAverageQueryToolChest.<init>(MovingAverageQueryToolChest.java:62)
  at org.apache.druid.query.movingaverage.MovingAverageQueryModule.configure(MovingAverageQueryModule.java:48) (via modules: com.google.inject.util.Modules$OverrideModule -> org.apache.druid.query.movingaverage.MovingAverageQueryModule)

because without the extension loaded, the router is unable to perform serde on the query making any queries that use this extension unable to use the router if it's not loaded:

Screen Shot 2019-07-29 at 8 24 52 PM

I'm unsure how to resolve this situation, i guess the router could inject null as a segment walker since none of this stuff will be called, it just needs the types to do serde, but that also seems sort of lame? Not sure what is best to do.

@clintropolis clintropolis modified the milestones: 0.15.1, 0.16.0 Jul 30, 2019
@clintropolis
Copy link
Copy Markdown
Member

I think because of the above reason, and because 0.16.0 is right around the corner, I am going to go ahead and make druid-0.15.1-incubating-rc1 without this patch.

@yurmix
Copy link
Copy Markdown
Contributor Author

yurmix commented Jul 30, 2019

@clintropolis were the only issues found are on services other than the broker?
If so, this is because it is purpusfully compatible only with the Broker, and the documentation states so:
To use this extension, make sure to load druid-moving-average-query only to the Broker.
I know other extensions have such guidelines (i.e. materialized-view).

I think since the fix solves a blocking bug and doesn't add any behavior change, it would be a good candidate for a minor release. also, the change is isolated to the extension code.

@clintropolis
Copy link
Copy Markdown
Member

@clintropolis were the only issues found are on services other than the broker?
If so, this is because it is purpusfully compatible only with the Broker, and the documentation states so:
To use this extension, make sure to load druid-moving-average-query only to the Broker.
I know other extensions have such guidelines (i.e. materialized-view).

I think since the fix solves a blocking bug and doesn't add any behavior change, it would be a good candidate for a minor release. also, the change is isolated to the extension code.

I guess it is true the docs mention that only the broker is supported, I'll admit I am unfamiliar with this extension 😅. The docs probably should be updated to mention that it will not work if you query through a router at some point in the future (doesn't need to be this PR).

I did just finish building an rc1 tag... but since I didn't push anything yet I will go ahead and +1 and do a backport once this is merged.

@clintropolis clintropolis modified the milestones: 0.16.0, 0.15.1 Jul 30, 2019
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurmix thanks for fixing this! Haven't had enough time to dig this issue.

@yurmix
Copy link
Copy Markdown
Contributor Author

yurmix commented Jul 30, 2019

I guess it is true the docs mention that only the broker is supported, I'll admit I am unfamiliar with this extension 😅. The docs probably should be updated to mention that it will not work if you query through a router at some point in the future (doesn't need to be this PR).

Thank you!
I will see what can I do in the future to make the extension load on at least all services in the Query server.

@lamberken
Copy link
Copy Markdown
Member

This issus still exist in 0.17.0
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Druid moving average query results in circular reference error

6 participants