Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement LookupMetadataLoadTask #620

Merged
merged 3 commits into from
Feb 12, 2018
Merged

Implement LookupMetadataLoadTask #620

merged 3 commits into from
Feb 12, 2018

Conversation

QubitPi
Copy link
Contributor

@QubitPi QubitPi commented Jan 31, 2018 •
edited
Loading

Addresses issue #624

@QubitPi QubitPi force-pushed the implement-lookup-load-task branch from 4190dc7 to 72be3ea Compare January 31, 2018 08:11
michael-mclawhorn
michael-mclawhorn requested changes Feb 1, 2018
View reviewed changes
fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java Outdated
@@ -306,6 +310,13 @@ protected void configure() {
setupDataSourceMetaData(healthCheckRegistry, dataSourceMetadataLoader);
}

if (DRUID_LOOKUP.isOn()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

change to druid_lookup_metadata to distinguish from a fact lookup feature.

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java Outdated
@@ -306,6 +310,13 @@ protected void configure() {
setupDataSourceMetaData(healthCheckRegistry, dataSourceMetadataLoader);
}

if (DRUID_LOOKUP.isOn()) {
setupLookUp(
healthCheckRegistry,
Copy link
Contributor

Choose a reason for hiding this comment

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

setupLookupMetadataLoader I think to disambiguate.

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java Outdated
setupLookUp(
healthCheckRegistry,
buildLookupLoader(metadataDruidWebService, loader.getDimensionDictionary())
);
Copy link
Contributor

Choose a reason for hiding this comment

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

lookupMetadaLoader

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java Outdated
@@ -726,6 +737,19 @@ protected DataSourceMetadataLoadTask buildDataSourceMetadataLoader(
);
}

/**
* Builds a lookup loader.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

lookup metadata loader

fili-core/src/main/java/com/yahoo/bard/webservice/config/BardFeatureFlag.java Outdated
@@ -15,6 +15,7 @@
INTERSECTION_REPORTING("intersection_reporting_enabled"),
UPDATED_METADATA_COLLECTION_NAMES("updated_metadata_collection_names_enabled"),
DRUID_COORDINATOR_METADATA("druid_coordinator_metadata_enabled"),
DRUID_LOOKUP("druid_lookup_enabled"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Druid_lookup_metadata

fili-core/src/main/java/com/yahoo/bard/webservice/metadata/LookupLoadTask.java Outdated
/**
* Lookup Load task sends requests to Druid coordinator and returns list of configured lookup statuses in Druid.
*/
public class LookupLoadTask extends LoadTask<Boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

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

LookupMetadata

fili-core/src/main/java/com/yahoo/bard/webservice/metadata/LookupLoadTask.java Outdated
lookupStatuses.put(entry.getKey(), entry.getValue().get("loaded").asBoolean());
}

unloadedLookups = dimensionDictionary.findAll().stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

There are three states on lookups, loaded, toLoad and toDrop. I think 'unloaded' is confusing because we usually mean toLoad but it sounds like they're being dropped. how about 'pending' where you have 'unloaded'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

@QubitPi QubitPi force-pushed the implement-lookup-load-task branch from f6a8a99 to 8099a96 Compare February 2, 2018 00:06
@QubitPi QubitPi changed the title Implement LookupLoadTask Implement LookupMetadataLoadTask Feb 2, 2018
michael-mclawhorn
michael-mclawhorn requested changes Feb 7, 2018
View reviewed changes
Copy link
Contributor

@michael-mclawhorn michael-mclawhorn left a comment

Choose a reason for hiding this comment

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

The only big change is to make the lookup namespace configurable. Multiple namespaces should be supported via a command delimited string config parameter.

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java Outdated
* @param dimensionDictionary A {@link com.yahoo.bard.webservice.data.dimension.DimensionDictionary} that is used
* to obtain a list of lookups in Fili.
*
* @return a lookup loader
Copy link
Contributor

Choose a reason for hiding this comment

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

lookup metadata loader

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java Outdated
@@ -768,6 +795,20 @@ protected final void setupDataSourceMetaData(
healthCheckRegistry.register(HEALTH_CHECK_NAME_DATASOURCE_METADATA, dataSourceMetadataLoaderHealthCheck);
}

/**
* Schedule a lookup loader and register its health check.
Copy link
Contributor

Choose a reason for hiding this comment

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

lookup metadata loader

...core/src/main/java/com/yahoo/bard/webservice/application/healthchecks/LookupHealthCheck.java Outdated
*/
@Singleton
public class LookupHealthCheck extends HealthCheck {
private final LookupMetadataLoadTask lookupMetadataLoadTask;
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this is potentially modified in another thread, we may need to make this volatile or make it a reference.

fili-core/src/main/java/com/yahoo/bard/webservice/metadata/LookupMetadataLoadTask.java Outdated
/**
* Location of lookup statuses on Druid coordinator.
*/
public static final String LOOKUP_QUERY = "/lookups/status/__default";
Copy link
Contributor

Choose a reason for hiding this comment

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

__default namespace is not always going to be where lookups are loaded. We need to make a list of namespaces that is configurable, defaulting to __default if not configured.

@QubitPi QubitPi force-pushed the implement-lookup-load-task branch from 0dd676a to 058e15b Compare February 8, 2018 18:28
@QubitPi QubitPi force-pushed the implement-lookup-load-task branch from 058e15b to 7500070 Compare February 8, 2018 18:28
@yahoo yahoo deleted a comment Feb 8, 2018
@QubitPi QubitPi force-pushed the implement-lookup-load-task branch from 41b4bcf to 8556d5c Compare February 8, 2018 20:30
@yahoo yahoo deleted a comment Feb 8, 2018
michael-mclawhorn
michael-mclawhorn approved these changes Feb 9, 2018
View reviewed changes
fili-core/src/main/resources/moduleConfig.properties Outdated
# A comma separated list of configured lookup tiers. See http://druid.io/docs/latest/querying/lookups.html
# The default is a list of 1 tier called "__default"
# If you have multiple tiers, write them like "tier1,tier2,tier3"
bard__lookup_tiers=__default
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make this explicit as druid_registered_lookup_tiers

@QubitPi
Copy link
Contributor Author

QubitPi commented Feb 10, 2018

Thanks for the approval, @michael-mclawhorn !

garyluoex
garyluoex approved these changes Feb 10, 2018
View reviewed changes
Copy link
Collaborator

@garyluoex garyluoex left a comment

Choose a reason for hiding this comment

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

@michael-mclawhorn I am ok with this PR, but have concern whether this will significantly reduce the availability of webservice. If any of the lookup is not loaded, webservice will not be up, and I am afraid that this means if a single lookup fail to load for some reason, the whole webservice is down.

@QubitPi
Copy link
Contributor Author

QubitPi commented Feb 12, 2018

@garyluoex I assume this is the problem we are addressing in this PR. We don't want webservice to be running with a broken lookup online. @michael-mclawhorn ?

@QubitPi
Copy link
Contributor Author

QubitPi commented Feb 12, 2018

@garyluoex Thank you for the approval!

@QubitPi QubitPi merged commit b84f494 into master Feb 12, 2018
@QubitPi QubitPi deleted the implement-lookup-load-task branch February 12, 2018 17:24
@QubitPi QubitPi mentioned this pull request Feb 12, 2018
Closed
@QubitPi QubitPi mentioned this pull request Mar 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyluoex garyluoex garyluoex approved these changes

@michael-mclawhorn michael-mclawhorn michael-mclawhorn approved these changes

Assignees
No one assigned
Labels
MERGEABLE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@QubitPi @michael-mclawhorn @garyluoex