Skip to content

Fixing the K8s task runner to work with MSQ - #13305

Merged
abhishekagarwal87 merged 2 commits into
apache:masterfrom
churromorales:k8s_and_msq
Nov 8, 2022
Merged

abhishekagarwal87 merged 2 commits into
apache:masterfrom
churromorales:k8s_and_msq

Conversation

@churromorales

Copy link
Copy Markdown
Contributor

MSQ can work with tasks that have not yet launched, there was a bug in the k8s task runner where you could return a null host because the pod was still not up. Fixed this to return a TaskLocation.unknown() if this happens to msq works. Tested this on our internal clusters and it seemed to fix the problem with msq + k8s peon jobs.

@churromorales

Copy link
Copy Markdown
Contributor Author

@gianm you were right, the bug was due to the worker task's pod being scheduled but it wasn't yet launched (so it had no podIP yet) thus, the task location was partially set. The fix is pretty small, I tested this on my local cluster and MSQ now works. Let me know what you think. Thanks for helping look into this.

@abhishekagarwal87

Copy link
Copy Markdown
Contributor

@churromorales - does this PR has all the changes pushed?

@churromorales

Copy link
Copy Markdown
Contributor Author

@abhishekagarwal87 it has even more, in our local fork i didn't write a test ;). Seriously though, here was the original issue:

#13156 (comment)

this fixes that issue, i tested and tasks index and i can query them, before it would not even start.

{
super(task.getId(), statusFuture, createdTime, createdTime);
this.task = task;
this.client = client;

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.

where does it get accessed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Used for testing. I needed to inject the client to mock out

@cryptoe cryptoe left a comment

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.

@churromorales Thanks for the PR.

I went through it and it seems we are not setting TaskLocation.unknown() anywhere.
Maybe I am missing something.

@churromorales

Copy link
Copy Markdown
Contributor Author

@churromorales - does this PR has all the changes pushed?

@abhishekagarwal87 I am so sorry, you are absolutely right, I missed the core logic here that was causing the issue. I forgot to push up the last commit. Now it should be good. I am very sorry about that.

@churromorales

Copy link
Copy Markdown
Contributor Author

@churromorales Thanks for the PR.

I went through it and it seems we are not setting TaskLocation.unknown() anywhere. Maybe I am missing something.

I forgot to push up a commit that was local. I apologize.

@abhishekagarwal87
abhishekagarwal87 merged commit 9a684af into apache:master Nov 8, 2022
@abhishekagarwal87

Copy link
Copy Markdown
Contributor

Thank you @churromorales

@kfaraz kfaraz added this to the 25.0 milestone Nov 22, 2022
riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
* Fixing the K8s task runner to work with MSQ

* Sorry incomplete PR

Co-authored-by: Rahul Gidwani <r_gidwani@apple.com>
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.

4 participants