Skip to content

[apiserver][feat] add pagination to ListRayJobs #3285

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

Conversation

owenowenisme
Copy link
Contributor

@owenowenisme owenowenisme commented Apr 8, 2025

Refer to #3269

Why are these changes needed?

Related issue number

Closes #3269

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@owenowenisme owenowenisme marked this pull request as ready for review April 8, 2025 16:06
@owenowenisme
Copy link
Contributor Author

@rueian PTAL

@owenowenisme
Copy link
Contributor Author

I will also open issues to add pagination to ListAllJob and ListAllCluster after this is merged.

@rueian
Copy link
Contributor

rueian commented Apr 8, 2025

Others LGTM. The test can be passed locally.

@owenowenisme
Copy link
Contributor Author

Thanks @rueian !
@kevin85421 this can be merged.

@kevin85421
Copy link
Member

cc @dentiny would you mind taking a look? Thanks!

@owenowenisme owenowenisme changed the title Api server add pagination to list ray jobs [apiserver][feat] add pagination to ListRayJobs Apr 9, 2025
@owenowenisme owenowenisme requested review from rueian and dentiny April 11, 2025 18:36
@owenowenisme
Copy link
Contributor Author

@dentiny

go test ./test/e2e/...  -timeout 60m  -race -count=1 -parallel 4
ok      github.com/ray-project/kuberay/apiserver/test/e2e    1438.115s

@@ -505,7 +572,7 @@ func createTestJob(t *testing.T, tCtx *End2EndTestingContext) *api.CreateRayJobR
configMapName := tCtx.CreateConfigMap(t, map[string]string{
"counter_sample.py": ReadFileAsString(t, "resources/counter_sample.py"),
"fail_fast.py": ReadFileAsString(t, "resources/fail_fast_sample.py"),
})
}, tCtx.currentName) // set the name of the config map to the current name
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why do we need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I added this because create configmap with same name will get error, but I just changed to the same way that we specify jobname.

for i := 0; i < testJobNum; i++ {
tCtx.currentName = fmt.Sprintf("job%d", i)
tCtx.configMapName = fmt.Sprintf("job%d-cm", i)
testJobs = append(testJobs, createTestJob(t, tCtx))
}

Copy link
Contributor

@dentiny dentiny left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all the comments!

@owenowenisme owenowenisme force-pushed the api-server-add-pagination-to-listRayJobs branch from 425d9f9 to 7af2743 Compare April 12, 2025 11:40
@kevin85421 kevin85421 merged commit 7f77346 into ray-project:master Apr 12, 2025
21 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.

[apiserver][feat] add pagination to ListRayJobs
4 participants