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

Druid v0.6 compatibility #2331

Closed
wants to merge 5 commits into from
Closed

Druid v0.6 compatibility #2331

wants to merge 5 commits into from

Conversation

dimaslv
Copy link

@dimaslv dimaslv commented Mar 3, 2017

These two commits allowed me to set up latest superset with our company's Druid v0.6 cluster. I have tried not to break anything for any newer versions.

@@ -1661,7 +1661,11 @@ def get_druid_version(self):
endpoint = (
"http://{obj.coordinator_host}:{obj.coordinator_port}/status"
).format(obj=self)
return json.loads(requests.get(endpoint).text)['version']
ver = json.loads(requests.get(endpoint).text)['version']
Copy link
Member

@mistercrunch mistercrunch Mar 3, 2017

Choose a reason for hiding this comment

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

I'd prefer an approach where we can specify the druid_version as a configuration element. I would have to be associated with the DruidCluster model. Here we'd use the information defined manually and fallback on this current approach when not specified.

Copy link
Author

@dimaslv dimaslv Mar 9, 2017

Choose a reason for hiding this comment

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

My programmer skills are quite weak and my knowledge about superset architecture is even more weak. Can you point me on how do I do that? Or maybe you can help me by coding this?

json=json.dumps({
'type': mt, 'name': name, 'fieldName': self.column_name})
))
if ver > '0':
Copy link
Member

@mistercrunch mistercrunch Mar 3, 2017

Choose a reason for hiding this comment

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

I'd rather take an approach like < '0.7.' here

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 9e5678d

'name': name,
'fn': '/',
'fields': [
{ 'type': 'fieldAccess', 'fieldName': 'sum__' + self.column_name },
Copy link
Member

Choose a reason for hiding this comment

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

Lint:
no space after {
no space before }
80 char max

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 18ba45b

@mistercrunch
Copy link
Member

Wouldn't it make more sense for this logic to be handled in pydruid?

@dimaslv
Copy link
Author

dimaslv commented Mar 13, 2017

I don't know anything about pydruid, but actually I don't see any "*avg" aggregations in latest Druid at all, so it's strange that you use such aggregation in you code. Could you explain please?
Can you please name minimal requirements for my pull request to be accepted?

@mistercrunch
Copy link
Member

There's been merge conflicts for a long time, feel free to re-open once conflicts are sorted out.

@dimaslv
Copy link
Author

dimaslv commented Aug 11, 2017

As I have not received answers to my questions for 5 months, I assume that you are not interested in those patches. Once anyone will speak to me again I will try to resume my work.

@mistercrunch
Copy link
Member

Sorry it's hard to keep up with so many PRs, hopefully we can ramp up more committers/maintainers soon.

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