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.

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

add druid client #39

Merged
merged 1 commit into from
Feb 6, 2018
Merged

add druid client #39

merged 1 commit into from
Feb 6, 2018

Conversation

danfrankj
Copy link
Collaborator

@danfrankj danfrankj commented Feb 6, 2018

@matthewwardrop Druid now supports SQL

self.__druid = connect(self.host, self.port, path='/druid/v2/sql/', scheme='http')

def _is_connected(self):
return hasattr(self, '__druid') and self.__druid is not None
Copy link
Collaborator

Choose a reason for hiding this comment

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

hasattr doesn't seem to be necessary here, given that it is explicitly set in the init method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

udpated!

Copy link
Collaborator

@matthewwardrop matthewwardrop left a comment

Choose a reason for hiding this comment

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

This looks good to me! Nice contribution.

I left a few comments where the client could be plausibly improved, but hardly anything worth worrying about 👍

logger.info('Disconnecting from Druid database ...')
try:
self.__druid.close()
except Exception:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know what the Druid client raises if it is not connected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's sometimes buggy so I'm going to leave this as is for now
druid-io/pydruid#119

self.__druid = connect(self.host, self.port, path='/druid/v2/sql/', scheme='http')

def _is_connected(self):
return hasattr(self, '__druid') and self.__druid is not None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does druid have a way to check if it is connected?

@danfrankj danfrankj force-pushed the df_add_druid_client branch from 1c56b67 to 32813bb Compare February 6, 2018 22:08
@danfrankj danfrankj force-pushed the df_add_druid_client branch from 32813bb to 3a89008 Compare February 6, 2018 22:31
@danfrankj danfrankj merged commit 9e5e01b into master Feb 6, 2018
@danfrankj danfrankj deleted the df_add_druid_client branch February 6, 2018 22:40
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.

2 participants