Skip to content

Fix task report streaming in https setups.#11739

Merged
clintropolis merged 3 commits intoapache:masterfrom
gianm:fix-task-report-https
Oct 23, 2021
Merged

Fix task report streaming in https setups.#11739
clintropolis merged 3 commits intoapache:masterfrom
gianm:fix-task-report-https

Conversation

@gianm
Copy link
Contributor

@gianm gianm commented Sep 24, 2021

TaskRunnerUtils.makeTaskLocationURL, which is only used by task report streaming, was not checking the https port. In common https-only setups, http is disabled, meaning task report streaming would not work at all.

return tlsPort;
}

public URL makeURL(final String encodedPathAndQueryString) throws MalformedURLException
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would makeUrl be more camel-casey?
Although, the community as well as the Druid repo seems a little divided on this. 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There doesn't seem to be much consistency here, like you said 🤠

I went with URL since there are so many other URLs in the same line (java.net.URL, MalformedURLException) so it seemed reasonable.

@clintropolis clintropolis merged commit cb9bc15 into apache:master Oct 23, 2021
@clintropolis
Copy link
Member

failing CI is the k8s test, which should be resolved by #11832

@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
@gianm gianm deleted the fix-task-report-https branch September 23, 2022 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants