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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/master.yml
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ jobs:
TEST_MODULES: "-pl :submarine-server-core"
run: |
echo ">>> mvn $TEST_FLAG $TEST_MODULES -B"
export SUBMARINE_S3_ENDPOINT=http://localhost:9000
mvn $TEST_FLAG $TEST_MODULES -B
- name: Build submarine-serve
env:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ private ModelManager(Submitter submitter, ModelVersionService modelVersionServic
*
* @return object
*/
private static class ModelManagerHolder {
private static ModelManager manager = new ModelManager(SubmitterManager.loadSubmitter(),
ModelVersionService.getInstance());
}

public static ModelManager getInstance() {
if (manager == null) {
synchronized (ModelManager.class) {
manager = new ModelManager(SubmitterManager.loadSubmitter(), new ModelVersionService());
}
}
return manager;
return ModelManager.ModelManagerHolder.manager;
}

/**
Expand Down Expand Up @@ -141,7 +141,7 @@ private void setServeInfo(ServeSpec spec, ModelVersionEntity modelVersion){
}

private void transferDescription(ServeSpec spec) {
Client s3Client = new Client();
Client s3Client = Client.getInstance();
String modelUniquePath = String.format("%s-%d-%s",
spec.getModelName(), spec.getModelVersion(), spec.getModelId());
String res = new String(s3Client.downloadArtifact(String.format("registry/%s/%s/%d/description.json",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Client.getInstance());
}

public static ModelVersionManager getInstance() {
if (manager == null) {
synchronized (ModelVersionManager.class) {
if (manager == null) {
manager = new ModelVersionManager(new ModelVersionService(), new ModelVersionTagService(),
new Client());
}
}
}
return manager;
return ModelVersionManager.ModelVersionManagerHolder.manager;
}

@VisibleForTesting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
* Registered model manager.
*/
public class RegisteredModelManager {
private static RegisteredModelManager manager;
/* Registered model service */
private final RegisteredModelService registeredModelService;

Expand All @@ -54,16 +53,17 @@ public class RegisteredModelManager {
*
* @return object
*/

private static class RegisteredModelManagerHolder {
private static RegisteredModelManager manager = new RegisteredModelManager(
RegisteredModelService.getInstance(),
ModelVersionService.getInstance(),
RegisteredModelTagService.getInstance(),
Client.getInstance());
}

public static RegisteredModelManager getInstance() {
if (manager == null) {
synchronized (RegisteredModelManager.class) {
if (manager == null) {
manager = new RegisteredModelManager(new RegisteredModelService(), new ModelVersionService(),
new RegisteredModelTagService(), new Client());
}
}
}
return manager;
return RegisteredModelManager.RegisteredModelManagerHolder.manager;
}

@VisibleForTesting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
@Produces({MediaType.APPLICATION_JSON + "; " + RestConstants.CHARSET_UTF8})
public class ExperimentRestApi {
private ExperimentManager experimentManager = ExperimentManager.getInstance();
private final Client minioClient = new Client();
private final Client minioClient = Client.getInstance();

@VisibleForTesting
public void setExperimentManager(ExperimentManager experimentManager) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,30 +45,26 @@
/**
* S3(Minio) default client
*/
public class Client {

/* minio client */
public MinioClient minioClient;
public enum Client {
INSTANCE(SubmarineConfiguration.getInstance().getString(SubmarineConfVars.ConfVars.SUBMARINE_S3_ENDPOINT));

/* submarine config */
private static final SubmarineConfiguration conf = SubmarineConfiguration.getInstance();

public Client() {
minioClient = MinioClient.builder()
.endpoint(conf.getString(SubmarineConfVars.ConfVars.SUBMARINE_S3_ENDPOINT))
.credentials(
conf.getString(SubmarineConfVars.ConfVars.SUBMARINE_S3_ACCESS_KEY_ID),
conf.getString(SubmarineConfVars.ConfVars.SUBMARINE_S3_SECRET_ACCESS_KEY)
).build();
private final SubmarineConfiguration conf = SubmarineConfiguration.getInstance();

/* minio client */
private final MinioClient minioClient;

Client(String endpoint) {
this.minioClient = MinioClient.builder()
.endpoint(endpoint)
.credentials(
conf.getString(SubmarineConfVars.ConfVars.SUBMARINE_S3_ACCESS_KEY_ID),
conf.getString(SubmarineConfVars.ConfVars.SUBMARINE_S3_SECRET_ACCESS_KEY)
).build();
}

public Client(String endpoint) {
minioClient = MinioClient.builder()
.endpoint(endpoint)
.credentials(
conf.getString(SubmarineConfVars.ConfVars.SUBMARINE_S3_ACCESS_KEY_ID),
conf.getString(SubmarineConfVars.ConfVars.SUBMARINE_S3_SECRET_ACCESS_KEY)
).build();
public static Client getInstance() {
return INSTANCE;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ public class ModelVersionRestApiTest {
private final String modelVersionModelType = "experiment_123";
private final String modelVersionTag = "testTag";

private final RegisteredModelService registeredModelService = new RegisteredModelService();
private final RegisteredModelService registeredModelService = RegisteredModelService.getInstance();

private final ModelVersionService modelVersionService = new ModelVersionService();
private final ModelVersionService modelVersionService = ModelVersionService.getInstance();

private static final GsonBuilder gsonBuilder = new GsonBuilder()
.registerTypeAdapter(ExperimentId.class, new ExperimentIdSerializer())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
import org.apache.submarine.server.utils.gson.ExperimentIdSerializer;

public class RegisteredModelRestApiTest {
private final RegisteredModelService registeredModelService = new RegisteredModelService();
private final RegisteredModelService registeredModelService = RegisteredModelService.getInstance();
private final String registeredModelName = "testRegisteredModel";
private final String newRegisteredModelName = "newTestRegisteredModel";
private final String registeredModelDescription = "test registered model description";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,33 @@

package org.apache.submarine.server.s3;

import org.apache.submarine.commons.utils.SubmarineConfVars;
import org.apache.submarine.commons.utils.SubmarineConfiguration;

import org.junit.BeforeClass;
import org.junit.After;
import org.junit.Assert;
import org.junit.Test;
import org.junit.Assert;

import java.util.List;


public class ClientTest {
private final Client client = new Client("http://localhost:9000");
private static Client client;
private final String testExperimentId = "experiment-sample";

@After
public void cleanAll() {
client.deleteAllArtifacts();
}

@BeforeClass
public static void init() {
SubmarineConfiguration conf = SubmarineConfiguration.getInstance();
conf.setString(SubmarineConfVars.ConfVars.SUBMARINE_S3_ENDPOINT, "http://localhost:9000");
client = Client.getInstance();
}

@Test
public void testLogArtifactAndDownloadArtifact() {
String path = "sample_folder/sample_file";
Expand Down Expand Up @@ -79,4 +90,10 @@ public void testCopyObject() {
response = client.downloadArtifact(copyPath);
Assert.assertArrayEquals(content, response);
}

@Test
public void testSingleton() {
Client testClient = Client.getInstance();
Assert.assertEquals(testClient, client);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@
import java.util.List;

public class ModelVersionService {
private static class ModelVersionServiceHolder {
private static ModelVersionService service = new ModelVersionService();
}

public static ModelVersionService getInstance() {
return ModelVersionService.ModelVersionServiceHolder.service;
}

private static final Logger LOG = LoggerFactory.getLogger(ModelVersionService.class);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@

public class RegisteredModelService {

private static class RegisteredModelServiceHolder {
private static RegisteredModelService service = new RegisteredModelService();
}

public static RegisteredModelService getInstance() {
return RegisteredModelService.RegisteredModelServiceHolder.service;
}

private static final Logger LOG = LoggerFactory.getLogger(RegisteredModelService.class);

public List<RegisteredModelEntity> selectAll() throws SubmarineRuntimeException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@
import org.slf4j.LoggerFactory;

public class RegisteredModelTagService {
private static class RegisteredModelTagServiceHolder {
private static RegisteredModelTagService service = new RegisteredModelTagService();
}

public static RegisteredModelTagService getInstance() {
return RegisteredModelTagService.RegisteredModelTagServiceHolder.service;
}

private static final Logger LOG = LoggerFactory.getLogger(RegisteredModelTagService.class);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@

public class ModelVersionTagTest {
private static final Logger LOG = LoggerFactory.getLogger(ModelVersionTagTest.class);
RegisteredModelService registeredModelService = new RegisteredModelService();
ModelVersionService modelVersionService = new ModelVersionService();
RegisteredModelService registeredModelService = RegisteredModelService.getInstance();
ModelVersionService modelVersionService = ModelVersionService.getInstance();
ModelVersionTagService modelVersionTagService = new ModelVersionTagService();


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@
import org.apache.submarine.server.database.model.service.RegisteredModelService;

public class ModelVersionTest {
RegisteredModelService registeredModelService = new RegisteredModelService();
ModelVersionService modelVersionService = new ModelVersionService();
RegisteredModelService registeredModelService = RegisteredModelService.getInstance();
ModelVersionService modelVersionService = ModelVersionService.getInstance();

@After
public void cleanAll() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import java.util.List;

public class RegisteredModelServiceTest {
RegisteredModelService registeredModelService = new RegisteredModelService();
RegisteredModelService registeredModelService = RegisteredModelService.getInstance();

@After
public void cleanAll() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@

public class RegisteredModelTagServiceTest {
private static final Logger LOG = LoggerFactory.getLogger(RegisteredModelTagServiceTest.class);
RegisteredModelService registeredModelService = new RegisteredModelService();
RegisteredModelTagService registeredModelTagService = new RegisteredModelTagService();
RegisteredModelService registeredModelService = RegisteredModelService.getInstance();
RegisteredModelTagService registeredModelTagService = RegisteredModelTagService.getInstance();

@After
public void cleanAll() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@

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

@SerializedName("kind")
private String kind;
private String kind = XGBoostJob.CRD_XGBOOST_KIND_V1 + "List";

@SerializedName("metadata")
private V1ListMeta metadata;
Expand All @@ -55,6 +55,6 @@ public String getApiVersion() {

@Override
public String getKind() {
return XGBoostJob.CRD_XGBOOST_KIND_V1 + "List";
return kind;
}
}