Skip to content

[QTL]Cached lookup module for JDBC connectors.#2819

Merged
drcrallen merged 1 commit intoapache:masterfrom
b-slim:new_lookup_module
Sep 16, 2016
Merged

[QTL]Cached lookup module for JDBC connectors.#2819
drcrallen merged 1 commit intoapache:masterfrom
b-slim:new_lookup_module

Conversation

@b-slim
Copy link
Copy Markdown
Contributor

@b-slim b-slim commented Apr 12, 2016 •
edited
Loading

In This PR introduces a cached Lookup leveraging eviction and various caching strategies.
This Lookup implementation is meant to be user with large Lookup tables that need evictions.

@b-slim b-slim force-pushed the new_lookup_module branch 2 times, most recently from a3ff3f2 to 6a926d4 Compare April 19, 2016 20:00
@b-slim b-slim changed the title [WIP][QTL]Cached lookup module for JDBC connectors. [QTL]Cached lookup module for JDBC connectors. Apr 19, 2016
himanshug
himanshug reviewed Apr 20, 2016
View reviewed changes
Comment thread extensions-core/druid-lookups/README.md Outdated
Copy link
Copy Markdown
Contributor

@himanshug himanshug Apr 20, 2016

Choose a reason for hiding this comment

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

can u move this doc to https://github.com/druid-io/druid/tree/master/docs/content/development/extensions-core and update https://github.com/druid-io/druid/blob/master/docs/content/development/extensions.md

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Apr 21, 2016

Choose a reason for hiding this comment

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

done

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Apr 21, 2016 

io.druid.indexing.overlord.RemoteTaskRunnerTest
testRunSameAvailabilityGroup(io.druid.indexing.overlord.RemoteTaskRunnerTest)  Time elapsed: 4.541 sec  <<< FAILURE!
java.lang.AssertionError: null
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)
    at org.junit.Assert.assertTrue(Assert.java:52)
    at io.druid.indexing.overlord.RemoteTaskRunnerTest.testRunSameAvailabilityGroup(RemoteTaskRunnerTest.java:232)

@b-slim b-slim closed this Apr 21, 2016
@b-slim b-slim reopened this Apr 21, 2016
himanshug
himanshug reviewed Apr 22, 2016
View reviewed changes
Comment thread extensions-core/druid-lookups/src/main/java/io/druid/server/lookup/DataFetcher.java Outdated
Copy link
Copy Markdown
Contributor

@himanshug himanshug Apr 22, 2016

Choose a reason for hiding this comment

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

s/none null/non-null

can you check in all places please, it seems repeated in many places.

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Apr 25, 2016

Choose a reason for hiding this comment

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

done

@himanshug
Copy link
Copy Markdown
Contributor

himanshug commented Jul 21, 2016

@b-slim can you please take care of the comments? lets get this merged.

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Jul 22, 2016

@himanshug sure ASAP.

@b-slim b-slim force-pushed the new_lookup_module branch from 0785b2f to 7887792 Compare July 29, 2016 20:00
@drcrallen
Copy link
Copy Markdown
Contributor

drcrallen commented Aug 4, 2016

@b-slim is this ready for review?

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Aug 16, 2016

@drcrallen and @himanshug this is ready for review, thanks

fjy
fjy reviewed Aug 30, 2016
View reviewed changes
Comment thread docs/content/development/extensions-core/druid-lookups.md Outdated
Copy link
Copy Markdown
Contributor

@fjy fjy Aug 30, 2016

Choose a reason for hiding this comment

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

this will not render correctly without

layout: doc_page

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Aug 31, 2016

Choose a reason for hiding this comment

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

thanks fixed !

@drcrallen
Copy link
Copy Markdown
Contributor

drcrallen commented Aug 31, 2016

Minor formatting changes and I suggest reverting LookupDimensionSpec unless the change is required for this PR and it is fully tested.

@drcrallen
Copy link
Copy Markdown
Contributor

drcrallen commented Aug 31, 2016

I think this extension will benefit more from battle testing by interested parties rather than further review. Good job @b-slim

gianm
gianm reviewed Sep 16, 2016
View reviewed changes
Comment thread docs/content/development/extensions-core/druid-lookups.md Outdated
Copy link
Copy Markdown
Contributor

@gianm gianm Sep 16, 2016

Choose a reason for hiding this comment

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

this would look prettier in a <div class="note info">xxx</div>

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Sep 16, 2016

Choose a reason for hiding this comment

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

fixed

Comment thread docs/content/development/extensions-core/druid-lookups.md Outdated
Copy link
Copy Markdown
Contributor

@gianm gianm Sep 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

no need for : on headings in general

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Sep 16, 2016

Choose a reason for hiding this comment

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

done

Comment thread ...obal/src/main/java/io/druid/server/lookup/namespace/JDBCExtractionNamespaceCacheFactory.java Outdated
Copy link
Copy Markdown
Contributor

@gianm gianm Sep 16, 2016

Choose a reason for hiding this comment

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

[nit] no need for this change

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Sep 16, 2016

Choose a reason for hiding this comment

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

weird i don't see this on my IDE

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Sep 16, 2016

Choose a reason for hiding this comment

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

ok i see it now i guess

Comment thread docs/content/development/extensions-core/druid-lookups.md Outdated
Copy link
Copy Markdown
Contributor

@gianm gianm Sep 16, 2016

Choose a reason for hiding this comment

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

Could you include something about whether it is intended that users could use this together with lookups-cached-global? Would be good to answer questions like:

  • Do they conflict with each other or can they be loaded simultaneously?
  • Does it make sense to run them side by side while migrating from one to the other?
  • Does it make sense to run them side by side long term?

Copy link
Copy Markdown
Contributor

@gianm gianm Sep 16, 2016

Choose a reason for hiding this comment

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

Could you also include how to set up this extension? Something like:

Make sure to [include](../../operations/including-extensions.html) `druid-lookups-cached-single` as an extension.

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Sep 16, 2016

Choose a reason for hiding this comment

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

good point will add

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Sep 16, 2016

Choose a reason for hiding this comment

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

done

Comment thread docs/content/development/extensions.md Outdated
Copy link
Copy Markdown
Contributor

@gianm gianm Sep 16, 2016

Choose a reason for hiding this comment

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

This should be druid-lookups-cached-single to match the actual module name of the extension.

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Sep 16, 2016

Choose a reason for hiding this comment

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

good catch

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Sep 16, 2016

Choose a reason for hiding this comment

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

done

gianm
gianm reviewed Sep 16, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

I didn't review the actual extension code, but just the docs and changes to code outside of the new extension. The comment about LookupDimensionSpec is blocking IMO; other than that I had a few suggestions for the docs.

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Sep 16, 2016

@gianm thanks fixed the doc and the blocking comment.

@drcrallen
Copy link
Copy Markdown
Contributor

drcrallen commented Sep 16, 2016

Gah! history got overwritten. ok, assuming only the comments got addressed and nothing else got changed then

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Sep 16, 2016

@drcrallen yes i did rebase it to revert and clean the dozen of commits messages.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Sep 16, 2016

👍 on the docs & non-extension changes. As discussed on the last dev sync, let's merge this (after travis) so people can give it a shot.

@drcrallen drcrallen merged commit 3175e17 into apache:master Sep 16, 2016
@drcrallen
Copy link
Copy Markdown
Contributor

drcrallen commented Sep 16, 2016

BOOM!

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Sep 16, 2016

good job, we made it to 300 comments on this PR :)

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Sep 16, 2016

oh such great PR 🐹

@gianm gianm mentioned this pull request Sep 23, 2016
Closed
@MdeArcayne MdeArcayne mentioned this pull request Sep 30, 2016
Closed
@b-slim b-slim deleted the new_lookup_module branch April 26, 2018 01:28
seoeun25 added a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020
@seoeun25 @navis
apache#2819 Add testAppliesTo at RuleTest
4a1ec1c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gianm gianm gianm left review comments

Assignees

No one assigned

Labels

Improvement

Projects

None yet

Milestone

0.9.2

Development

Successfully merging this pull request may close these issues.

6 participants

@b-slim @drcrallen @himanshug @gianm @fjy @erikdubbelboer