Skip to content

assume http if scheme is null#4912

Merged
pjain1 merged 1 commit intoapache:masterfrom
pjain1:fix_log_stream
Oct 6, 2017
Merged

assume http if scheme is null#4912
pjain1 merged 1 commit intoapache:masterfrom
pjain1:fix_log_stream

Conversation

@pjain1
Copy link
Copy Markdown
Member

@pjain1 pjain1 commented Oct 6, 2017

Fixes #4910

@pjain1 pjain1 added this to the 0.11.0 milestone Oct 6, 2017
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you update Worker class instead to handle null scheme in constructor so that its covered for all the places worker.getScheme() would have been used?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think handling it in the Worker class makes sense too.

Also that would fix other usages of worker.getScheme() that don't have null checks (looks like there is 1 other).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

changed

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.

thanks for patching so quickly @pjain1!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a comment that this is needed for backwards compatibility with older workers (pre-#4270) and I think this is good to go.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

added

@himanshug
Copy link
Copy Markdown
Contributor

👍

@pjain1 pjain1 merged commit 535c034 into apache:master Oct 6, 2017
@pjain1 pjain1 deleted the fix_log_stream branch October 6, 2017 19:50
leventov pushed a commit to metamx/druid that referenced this pull request Oct 6, 2017
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.

3 participants