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 port_count=1 to DQ job in YT #13459

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Krock21
Copy link
Collaborator

@Krock21 Krock21 commented Jan 16, 2025

Changelog category

  • Not for changelog (changelog entry is not required)

Additional information

Currently, DQ job binds on some port and expects it to be reachable from yql agent

Ideally, it should declare port_count = 1 and use the port provided by exec node

This PR adds port_count = 1 to this job to prevent it from being placed on portless exec nodes (that do not have ports)

I created an issue to ytsaurus to start using this port: ytsaurus/ytsaurus#1034

@Krock21 Krock21 requested a review from Krisha11 January 16, 2025 18:25
@Krock21 Krock21 requested a review from a team as a code owner January 16, 2025 18:25
Copy link

github-actions bot commented Jan 16, 2025

2025-01-16 18:28:59 UTC Pre-commit check linux-x86_64-release-asan for 8379d38 has started.
2025-01-16 18:29:10 UTC Artifacts will be uploaded here
2025-01-16 18:31:31 UTC ya make is running...
🟢 2025-01-16 18:33:04 UTC Tests successful.

Test history | Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
9 9 0 0 0 0

🟢 2025-01-16 18:33:11 UTC Build successful.

Copy link

github-actions bot commented Jan 16, 2025

2025-01-16 18:29:00 UTC Pre-commit check linux-x86_64-relwithdebinfo for 8379d38 has started.
2025-01-16 18:29:12 UTC Artifacts will be uploaded here
2025-01-16 18:31:36 UTC ya make is running...
🟢 2025-01-16 18:32:51 UTC Tests successful.

Test history | Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
9 9 0 0 0 0

🟢 2025-01-16 18:32:57 UTC Build successful.

@@ -678,6 +678,7 @@ namespace NYql {
fluent.Item("file_paths").Value(*filePaths);
})
.Item("job_count").Value(1)
.Item("port_count").Value(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Port requesting doesn't work like that. You can't specify port range that you need (and it is not clear why an user would want to specify that)

You just specify port_count = 1 and the job will have YT_PORT_0 env variable set with a port assigned. This port will be from a range configured in exec nodes static config and may be different for different exec nodes

This PR doesn't use this port, it only requests one to avoid placing on nodes with 0 available ports, since they are more likely to be unavailable on 31002

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.

2 participants