Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-1349. Fix the syntax error reported in sonarcloud and add init value for apiversion in XGboostjobList.java #1020

Closed
wants to merge 17 commits into from
Closed

SUBMARINE-1349. Fix the syntax error reported in sonarcloud and add init value for apiversion in XGboostjobList.java #1020

wants to merge 17 commits into from

Conversation

hhcs9527
Copy link
Contributor

@hhcs9527 hhcs9527 commented Nov 22, 2022 •
edited
Loading

What is this PR for?

  1. Fix the syntax error reported in sonarcloud
  2. Add init value for apiversion in XGboostjobList.java
  3. change the following class to a singleton, ModelVersionService.java, RegisteredModelService.java,RegisteredModelTagService.java, and the managers used thems.

What type of PR is it?

Bug Fix

Todos

  • - Task

What is the Jira issue?

SUBMARINE-1349

How should this be tested?

Screenshots (if appropriate)

Questions:

  • Do the license files need updating? No
  • Are there breaking changes for older versions? No
  • Does this need new documentation? No

@hhcs9527
Copy link
Contributor Author

Hi @cdmikechen , Please take a look at this PR, thanks.

@codecov
Copy link

codecov bot commented Nov 22, 2022 •
edited
Loading

Codecov Report

Merging #1020 (fad6b5a) into master (8360aec) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1020   +/-   ##
=======================================
  Coverage   75.98%   75.98%           
=======================================
  Files         119      119           
  Lines        5000     5000           
=======================================
  Hits         3799     3799           
  Misses       1201     1201
Flag Coverage Δ
python-integration 59.72% <ø> (ø)
python-unit 52.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cdmikechen
cdmikechen suggested changes Nov 23, 2022
View reviewed changes
...erver/server-core/src/main/java/org/apache/submarine/server/manager/ModelVersionManager.java Outdated Show resolved Hide resolved
...er/server-core/src/main/java/org/apache/submarine/server/manager/RegisteredModelManager.java Outdated
@@ -54,14 +54,10 @@ public class RegisteredModelManager {
*
* @return object
*/
public static RegisteredModelManager getInstance() {
public static synchronized RegisteredModelManager getInstance() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

...src/main/java/org/apache/submarine/server/submitter/k8s/model/xgboostjob/XGBoostJobList.java Outdated
@@ -27,7 +27,7 @@

public class XGBoostJobList implements KubernetesListObject {
@SerializedName("apiVersion")
private String apiVersion;
private String apiVersion = XGBoostJob.CRD_XGBOOST_API_VERSION_V1;

@SerializedName("kind")
private String kind;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does kind have the same problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdmikechen kind doesn't show the problem in sonarcloud.

@hhcs9527
Copy link
Contributor Author

@cdmikechen just updated with some modifications, please take a look. (singleton, and their reference)

@cdmikechen
Copy link
Contributor

@hhcs9527
There are some check style and build task errors, please take care of this part first, thanks~

cdmikechen
cdmikechen reviewed Dec 10, 2022
View reviewed changes
submarine-server/server-core/src/main/java/org/apache/submarine/server/s3/Client.java Outdated
public static Map<String,MinioClient> minioClientFactory = new HashMap<String, MinioClient>();
public static Map<String,Client> clientFactory = new HashMap<String, Client>();

public static Client getClient(String endpoint) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can use a enum class to replace inner factory class to avoid client double checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cdmikechen , I agree with you.
But the enum type is more suitable for the predefined class (no argument to init). In our case, we need to init different users in testing, which cannot be predefined; that's why I use double-check locking.

Copy link
Contributor

@cdmikechen cdmikechen Dec 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Java enum class can be used inside itself to create a constructor with parameters and as a singleton. Variables are only one of its basic uses.

@hhcs9527
Copy link
Contributor Author

hhcs9527 commented Dec 19, 2022 •
edited
Loading

Hi @cdmikechen just updated Java enum class, please take a look

cdmikechen
cdmikechen suggested changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@cdmikechen cdmikechen left a comment

Choose a reason for hiding this comment

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

codecov currently finds that the current changes do not have some test cases, please add some test cases for the minio client section. The other sections can be left alone for now.

...erver/server-core/src/main/java/org/apache/submarine/server/manager/ModelVersionManager.java
@@ -50,16 +50,14 @@ public class ModelVersionManager {
*
* @return object
*/
private static class ModelVersionManagerHolder {
private static ModelVersionManager manager = new ModelVersionManager(ModelVersionService.getInstance(),
new ModelVersionTagService(),
Copy link
Contributor

Choose a reason for hiding this comment

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

new class param should also be avoided here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cdmikechen ,
Not sure what you mean by adding some test cases for the minio client section here.

Copy link
Contributor

Choose a reason for hiding this comment

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

New code changes need to be supported by test cases, otherwise they will reduce code coverage and fail to prove code robustness.

submarine-server/server-core/src/main/java/org/apache/submarine/server/s3/Client.java Outdated
@@ -45,30 +47,57 @@
/**
* S3(Minio) default client
*/
public class Client {
public enum Client {
DEFAULT(SubmarineConfiguration.getInstance().getString(SubmarineConfVars.ConfVars.SUBMARINE_S3_ENDPOINT)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to add an endpoint attribute to the enum, which can't declare a variable number of endpoints and has to use the internal map to handle it.

public enum Client {
  INSTANCE;
  public static Client getInstance(String endpoint) {
    // ....
  }
}

Client.INSTANCE.getInstance("http://localhost:9000");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cdmikechen ,
Not sure what you mean here as well. Are you suggesting not initiating the enum variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

When using an enum as a factory to get a single instance object, using a limited number of enum definitions does not achieve the final effect of a dynamic factory

cdmikechen
cdmikechen suggested changes Jan 16, 2023
View reviewed changes
Copy link
Contributor

@cdmikechen cdmikechen left a comment

Choose a reason for hiding this comment

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

Hi~ There are still some test case failed and we can merge when you have resolved this issue.

@hhcs9527
Copy link
Contributor Author

@cdmikechen we still facing the same error for being unable to connect to the local host.
Any good suggestion?

cdmikechen
cdmikechen approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@cdmikechen cdmikechen left a comment

Choose a reason for hiding this comment

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

LGTM
I will merge this pr after python cicd fixed

@hhcs9527 hhcs9527 force-pushed the SUBMARINE-1349 branch 3 times, most recently from 815c0a4 to 9e05aa1 Compare February 24, 2023 06:10
@hhcs9527 hhcs9527 closed this Mar 5, 2023
@hhcs9527 hhcs9527 reopened this Mar 5, 2023
@asfgit asfgit closed this in 279393a Mar 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cdmikechen cdmikechen cdmikechen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hhcs9527 @cdmikechen