jiwq edited a comment on issue #249: SUBMARINE-455. Support find/patch/delete job in submarine-server REST URL: https://github.com/apache/submarine/pull/249#issuecomment-604933388 Thanks @liuxunorg and @yuanzac reviewing. I will complete the unit test as soon later. > Please change var name `jobIdString` to `jobId`. Good catch, considering the future develop maybe we can use the `id` to replace the `jobIdString`. Because `id` looks simple and cleaner, and its meaning is more intuitive. > Should we need deleted the tf_job_mnist.json and pytorch_job_mnist_gloo.json files? Yes, we used the submarine job spec file replace it. By the way the `k8s submitter` only supports the `JobSpec` as the input param. So we don't need this files no longer. > Since K8sJobSubmitter is public, we don't need to add "@VisibleForTesting" Good catch. Due to the `SubmitterManager` only supports non params constructor, so I think we should add the annotation to indicate the method only used in the test code. Thoughts? > We'd better change findJob() to getJob(), as jobhandler uses getJob(). In my opinion, used the `find` prefix is better that `get`, because we often use the `get` prefix in the getter method. Any thoughts?
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
