Skip to content

Fix code_search_path failed in java#11406

Merged
kfstorm merged 4 commits intoray-project:masterfrom
Xuxue1:fix_ray.job.code-search-path
Oct 21, 2020
Merged

Fix code_search_path failed in java#11406
kfstorm merged 4 commits intoray-project:masterfrom
Xuxue1:fix_ray.job.code-search-path

Conversation

@Xuxue1
Copy link
Copy Markdown
Contributor

@Xuxue1 Xuxue1 commented Oct 15, 2020 •
edited
Loading

Why are these changes needed?

fix code_search_path failed in java

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@Xuxue1 Xuxue1 changed the title fix code_search_path failed in java Fix code_search_path failed in java Oct 15, 2020
ffbin
ffbin reviewed Oct 16, 2020
View reviewed changes
src/ray/raylet/worker_pool.cc
}
case Language::JAVA: {
code_search_path_str = "-Dray.job.code-search-path" + code_search_path_str;
code_search_path_str = "-Dray.job.code-search-path=" + code_search_path_str;
Copy link
Copy Markdown
Contributor

@ffbin ffbin Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am confused that it seems to be an obvious problem, but it hasn't happened before. Thanks.

Copy link
Copy Markdown
Contributor Author

@Xuxue1 Xuxue1 Oct 16, 2020

Choose a reason for hiding this comment

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

I run a demo in ray cluster and set code-search-path in ray.conf。 but it failed. I find start worker process command wrong.

@ffbin ffbin self-assigned this Oct 16, 2020
@ffbin ffbin added the java label Oct 16, 2020
ffbin
ffbin approved these changes Oct 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ffbin ffbin left a comment

Choose a reason for hiding this comment

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

LGTM

@kfstorm
Copy link
Copy Markdown
Member

kfstorm commented Oct 16, 2020

@Xuxue1 May I ask what exact failure message you encountered? And is it convenient for you to add a test case?

@chaokunyang The bug seems obvious. I don't know why we didn't catch it before.

@chaokunyang
Copy link
Copy Markdown
Member

chaokunyang commented Oct 16, 2020

@kfstorm This codepath only run for multi-tenancy. We need a test on multi-tenancy for it.

@kfstorm
Copy link
Copy Markdown
Member

kfstorm commented Oct 16, 2020

@chaokunyang But we already enabled multi-tenancy on the master branch.

@Xuxue1
Copy link
Copy Markdown
Contributor Author

Xuxue1 commented Oct 16, 2020

@kfstorm I run a demo in ray cluster and set code-search-path in ray.conf. It can't find class.

@Xuxue1
Copy link
Copy Markdown
Contributor Author

Xuxue1 commented Oct 16, 2020

@kfstorm Ok. I try to add test cases.

@kfstorm
Copy link
Copy Markdown
Member

kfstorm commented Oct 20, 2020

@Xuxue1 Hi, any update on this issue? If you have trouble adding a test case, maybe @chaokunyang can help.

@kfstorm
Copy link
Copy Markdown
Member

kfstorm commented Oct 20, 2020

@Xuxue1 Thanks for the update. There's still a lint error. BTW, you can run scripts/format.sh to automatically format code.

kfstorm
kfstorm approved these changes Oct 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@kfstorm kfstorm left a comment

Choose a reason for hiding this comment

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

LGTM.

@kfstorm kfstorm merged commit 7200ddb into ray-project:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfstorm kfstorm kfstorm approved these changes

@ffbin ffbin ffbin approved these changes

@chaokunyang chaokunyang Awaiting requested review from chaokunyang

@raulchen raulchen Awaiting requested review from raulchen

Assignees

@ffbin ffbin

Labels

java

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Xuxue1 @kfstorm @chaokunyang @ffbin