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

Replace UI/Non-UI broker configuration with one broker #489

Closed
wants to merge 13 commits into from
Closed

Replace UI/Non-UI broker configuration with one broker #489

wants to merge 13 commits into from

Conversation

mpardesh
Copy link
Contributor

@mpardesh mpardesh commented Aug 6, 2017

Here is an attempt at replacing the UI/non-UI code to simplify the code. This feature isn't used anymore and complicates the config. I left the UI and non-UI settings in the configuration for backwards compatibility. First, we will check if druid_broker is set. If not, we will try to use non_ui_druid_broker and then ui_druid_broker.

@yahoo yahoo deleted a comment Aug 6, 2017
@yahoo yahoo deleted a comment Aug 6, 2017
kevinhinterlong
kevinhinterlong requested changes Aug 7, 2017
View reviewed changes
Copy link
Member

@kevinhinterlong kevinhinterlong left a comment

Choose a reason for hiding this comment

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

Overall pretty good. Mostly minor fixes, but it looks like the AsyncDruidWebServiceImplSpec tests needs to be fixed up as well.

fili-core/src/main/java/com/yahoo/bard/webservice/druid/client/DruidClientConfigHelper.java Outdated
@@ -96,18 +128,46 @@ public static Integer getDruidNonUiPriority() {
* Fetches the druid UI URL.
*
* @return druid UI URL
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Add a newline after @return and then add a description saying to use {@link #getDruidUrl()}

fili-core/src/main/java/com/yahoo/bard/webservice/druid/client/DruidClientConfigHelper.java Outdated
return Integer.parseInt(priority);
}

/**
* Fetches the druid non-UI Priority.
*
* @return druid non-UI URL
* @return druid non-UI priority
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

same here

fili-core/src/main/java/com/yahoo/bard/webservice/druid/client/DruidClientConfigHelper.java Outdated
* The default timeout for queries.
*/
private static final int DRUID_REQUEST_TIMEOUT_DEFAULT = Math.toIntExact(TimeUnit.MINUTES.toMillis(10));

/**
* Fetches the druid UI request Priority.
*
* @return druid UI request timeout
* @return druid UI request priority
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

same here

fili-core/src/main/java/com/yahoo/bard/webservice/druid/client/DruidClientConfigHelper.java Outdated
}

/**
* Fetches the druid non-UI URL.
*
* @return druid non-UI URL
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

same here

fili-core/src/main/java/com/yahoo/bard/webservice/druid/client/DruidClientConfigHelper.java Outdated
@@ -123,7 +183,9 @@ public static String getDruidCoordUrl() {
* Fetches the druid UI request timeout.
*
* @return druid UI request timeout
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Same here

fili-core/src/main/java/com/yahoo/bard/webservice/druid/client/DruidClientConfigHelper.java Outdated
@@ -132,9 +194,28 @@ public static Integer getDruidUiTimeout() {
* Fetches the druid non-UI request timeout.
*
* @return druid non-UI request timeout
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Same here

fili-core/src/main/java/com/yahoo/bard/webservice/druid/client/DruidClientConfigHelper.java
@@ -155,6 +236,10 @@ public static DruidServiceConfig getNonUiServiceConfig() {
return new DruidServiceConfig("Broker", getDruidNonUiUrl(), getDruidNonUiTimeout(), getDruidNonUiPriority());
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't getNonUiServiceConfig() and getUiServiceConfig() be deprecated as well?

Also getNonUiServiceConfig() is used in AsyncDruidWebServiceImplSpec, which should probably be replaced with calls to getServiceConfig().

...ore/src/test/groovy/com/yahoo/bard/webservice/web/handlers/workflow/DruidWorkflowSpec.groovy Outdated
handlers = getHandlerChain(defaultHandler.nonUiWebServiceHandler.next)

then:
[AsyncWebServiceRequestHandler, DebugRequestHandler, WeightCheckRequestHandler].every {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we move WeightCheckRequestHandler up to the previous then section so that we're still testing that it's in the workflow?

...ore/src/test/groovy/com/yahoo/bard/webservice/web/handlers/workflow/DruidWorkflowSpec.groovy Outdated
@@ -242,12 +211,10 @@ class DruidWorkflowSpec extends Specification {
def defaultHandler = select.handlerSelector as DefaultWebServiceHandlerSelector

when:
def handlers1 = getHandlerChain(defaultHandler.uiWebServiceHandler.next)
def handlers2 = getHandlerChain(defaultHandler.nonUiWebServiceHandler.next)
def handlers1 = getHandlerChain(defaultHandler.webServiceHandler.next)
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the 1 from the variable name

kevinhinterlong
kevinhinterlong reviewed Aug 8, 2017
View reviewed changes
fili-core/src/main/java/com/yahoo/bard/webservice/web/handlers/workflow/DruidWorkflow.java Outdated
@@ -85,8 +83,7 @@
@Inject
public DruidWorkflow(
@NotNull DataCache<?> dataCache,
@Named("uiDruidWebService") DruidWebService uiWebService,
@Named("nonUiDruidWebService") DruidWebService nonUiWebService,
@Named("druidWebService") DruidWebService webService,
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to remove @Named("druidWebService") here and the call to .named("druidWebService") in AbstractBinderFactory since there's only one

@yahoo yahoo deleted a comment Aug 10, 2017
@yahoo yahoo deleted a comment Aug 10, 2017
kevinhinterlong
kevinhinterlong approved these changes Aug 11, 2017
View reviewed changes
Copy link
Member

@kevinhinterlong kevinhinterlong left a comment

Choose a reason for hiding this comment

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

Looks good. I pulled in a comment that I forgot to include in my last review, but it's pretty small

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java Outdated

bind(uiDruidWebService).named("uiDruidWebService").to(DruidWebService.class);
bind(nonUiDruidWebService).named("nonUiDruidWebService").to(DruidWebService.class);
bind(druidWebService).named("druidWebService").to(DruidWebService.class);
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to remove the call to .named("druidWebService") since there's only one. Then you can remove the annotations that have @Named("druidWebService")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to do this in my earlier commit. I have pushed another commit with these changes.

@yahoo yahoo deleted a comment Aug 11, 2017
@yahoo yahoo deleted a comment Aug 11, 2017
@archolewa
Copy link
Contributor

@mpardesh This guy needs to be rebased as well.

@yahoo yahoo deleted a comment Aug 24, 2017
@yahoo yahoo deleted a comment Aug 25, 2017
@yahoo yahoo deleted a comment Aug 25, 2017
@michael-mclawhorn michael-mclawhorn mentioned this pull request Aug 28, 2017
Merged
@michael-mclawhorn
Copy link
Contributor

Merged from another PR. Good job.

@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

@kevinhinterlong kevinhinterlong kevinhinterlong approved these changes

Assignees
No one assigned
Labels
NEED 1 REVIEW REVIEWABLE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mpardesh @archolewa @michael-mclawhorn @kevinhinterlong