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

Refuse relative path for remote in coordinator #538

Merged
merged 15 commits into from
Jun 12, 2024
Merged

Refuse relative path for remote in coordinator #538

merged 15 commits into from
Jun 12, 2024

Conversation

XxChang
Copy link
Collaborator

@XxChang XxChang commented Jun 6, 2024

Only allow absolute path for remote daemon in coordinator,

modified from #534

See option 2 in #535

@XxChang
Copy link
Collaborator Author

XxChang commented Jun 6, 2024

@Gege-Wang please help me check it in remote daemon

@Gege-Wang
Copy link
Collaborator

@XxChang Hi, I think the machine who executes dora start dataflow.yml is the local machine.In this PR, the local machine is same as the coordinator, right?

@haixuanTao
Copy link
Collaborator

So this work on aws EC2 right?

And now you should be able to spawn multiple-daemon example without using the same folder structure right?

@phil-opp
Copy link
Collaborator

I think the machine who executes dora start dataflow.yml is the local machine.In this PR, the local machine is same as the coordinator, right?

Good point!

@XxChang Could you add an additional check in dora start that the coordinator IP is local too? Then we can be sure that the coordinator and all relevant daemons run on the same machine.

@XxChang
Copy link
Collaborator Author

XxChang commented Jun 11, 2024

I think the machine who executes dora start dataflow.yml is the local machine.In this PR, the local machine is same as the coordinator, right?

Good point!

@XxChang Could you add an additional check in dora start that the coordinator IP is local too? Then we can be sure that the coordinator and all relevant daemons run on the same machine.

Sure, but if we do that. The "coordinator_addr" optional flag in dora start will be no sense

@phil-opp
Copy link
Collaborator

@XxChang Could you add an additional check in dora start that the coordinator IP is local too? Then we can be sure that the coordinator and all relevant daemons run on the same machine.

Sure, but if we do that. The "coordinator_addr" optional flag in dora start will be no sense

I meant as a requirement for absolute paths. Relative paths should be only allowed if the CLI, the coordinator, and the daemon(s) are all on the same machine.

So you can still use --coordinator_addr, but not in combination with relative paths.

@XxChang
Copy link
Collaborator Author

XxChang commented Jun 12, 2024

Add additional check in dora start. Only allow relative path to be used in the case that dora-cli and dora-coordinator are in the same machine.

Also modify the ci.yml to left enough space for python-dataflow test case.

Copy link
Collaborator

@haixuanTao haixuanTao left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Thanks! Looks very good!

Maybe i will let Philipp have a look and merge it

Copy link
Collaborator

@phil-opp phil-opp 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 the updates and thanks for finding a way to free up enough space on CI!

I left one suggestion, otherwise this looks good to me.

libraries/core/src/descriptor/validate.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks!

@phil-opp phil-opp merged commit 799a3a6 into dora-rs:main Jun 12, 2024
18 checks passed
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