ofuks commented on a change in pull request #926:
URL: https://github.com/apache/incubator-datalab/pull/926#discussion_r505590648
##########
File path:
services/self-service/src/main/java/com/epam/dlab/backendapi/dao/OdahuDAOImpl.java
##########
@@ -46,6 +48,7 @@
import static com.mongodb.client.model.Updates.push;
import static java.util.stream.Collectors.toList;
+
Review comment:
Please remove this line
##########
File path:
services/self-service/src/main/java/com/epam/dlab/backendapi/dao/OdahuDAOImpl.java
##########
@@ -77,15 +80,20 @@
@Override
public OdahuFieldsDTO getFields(String name, String project, String
endpoint) {
- Optional<Document> one = findOne(PROJECTS_COLLECTION,
odahuProjectEndpointCondition(name, project, endpoint),
- fields(include(ODAHU_FIELD), excludeId()));
- OdahuFieldsDTO odahuFields = null;
- if (one.isPresent()) {
- List<OdahuFieldsDTO> list =
convertFromDocument(one.get().get(ODAHU_FIELD, ArrayList.class), new
TypeReference<List<OdahuFieldsDTO>>() {
- });
- odahuFields = list.get(0);
- }
- return odahuFields;
+ FindIterable<Document> odahuDoc = find(PROJECTS_COLLECTION,
+ odahuProjectEndpointCondition(name, project, endpoint),
+ fields(include(ODAHU_FIELD), excludeId())); //TODO:
Potentially, method can be improved by filtering the Documents
Review comment:
Could you please implement a method **find(String collection, Bson
condition, Bson projection, Class<T> resultedClass)** in BaseDAO class?
##########
File path:
services/self-service/src/main/java/com/epam/dlab/backendapi/dao/OdahuDAOImpl.java
##########
@@ -77,15 +80,20 @@
@Override
public OdahuFieldsDTO getFields(String name, String project, String
endpoint) {
- Optional<Document> one = findOne(PROJECTS_COLLECTION,
odahuProjectEndpointCondition(name, project, endpoint),
- fields(include(ODAHU_FIELD), excludeId()));
- OdahuFieldsDTO odahuFields = null;
- if (one.isPresent()) {
- List<OdahuFieldsDTO> list =
convertFromDocument(one.get().get(ODAHU_FIELD, ArrayList.class), new
TypeReference<List<OdahuFieldsDTO>>() {
- });
- odahuFields = list.get(0);
- }
- return odahuFields;
+ FindIterable<Document> odahuDoc = find(PROJECTS_COLLECTION,
+ odahuProjectEndpointCondition(name, project, endpoint),
+ fields(include(ODAHU_FIELD), excludeId())); //TODO:
Potentially, method can be improved by filtering the Documents
+
+ Optional<OdahuFieldsDTO> odahuFields;
+ List<OdahuFieldsDTO> odahuFieldsDTOList;
+
+ odahuFieldsDTOList =
convertFromDocument(odahuDoc.first().get(ODAHU_FIELD, ArrayList.class), new
TypeReference<List<OdahuFieldsDTO>>() {});
+ odahuFields = odahuFieldsDTOList.stream()
+ .filter(oDTO -> oDTO.getName().equals(name))
+ .findAny();
Review comment:
Need to check out whether we need to **findAny()** here. Let's discuss
this
##########
File path:
services/self-service/src/main/java/com/epam/dlab/backendapi/dao/OdahuDAOImpl.java
##########
@@ -77,15 +80,20 @@
@Override
public OdahuFieldsDTO getFields(String name, String project, String
endpoint) {
- Optional<Document> one = findOne(PROJECTS_COLLECTION,
odahuProjectEndpointCondition(name, project, endpoint),
- fields(include(ODAHU_FIELD), excludeId()));
- OdahuFieldsDTO odahuFields = null;
- if (one.isPresent()) {
- List<OdahuFieldsDTO> list =
convertFromDocument(one.get().get(ODAHU_FIELD, ArrayList.class), new
TypeReference<List<OdahuFieldsDTO>>() {
- });
- odahuFields = list.get(0);
- }
- return odahuFields;
+ FindIterable<Document> odahuDoc = find(PROJECTS_COLLECTION,
+ odahuProjectEndpointCondition(name, project, endpoint),
+ fields(include(ODAHU_FIELD), excludeId())); //TODO:
Potentially, method can be improved by filtering the Documents
+
+ Optional<OdahuFieldsDTO> odahuFields;
+ List<OdahuFieldsDTO> odahuFieldsDTOList;
+
+ odahuFieldsDTOList =
convertFromDocument(odahuDoc.first().get(ODAHU_FIELD, ArrayList.class), new
TypeReference<List<OdahuFieldsDTO>>() {});
+ odahuFields = odahuFieldsDTOList.stream()
+ .filter(oDTO -> oDTO.getName().equals(name))
+ .findAny();
+ if (odahuFields.isPresent()){
+ return odahuFields.get();
+ } else throw new DlabException("Unable to find the " + name + "
cluster properties");
Review comment:
Please do not inline **else** body.
##########
File path:
services/self-service/src/main/java/com/epam/dlab/backendapi/domain/OdahuFieldsDTO.java
##########
@@ -26,6 +26,8 @@
@Data
@JsonIgnoreProperties(ignoreUnknown = true)
public class OdahuFieldsDTO {
+ @JsonProperty("name")
Review comment:
Do we need it here?
##########
File path:
services/self-service/src/main/java/com/epam/dlab/backendapi/service/impl/OdahuServiceImpl.java
##########
@@ -132,14 +134,20 @@ public void stop(String name, String project, String
endpoint, UserInfo user) {
@Override
public void terminate(String name, String project, String endpoint,
UserInfo user) {
- Optional<OdahuDTO> odahuDTO = get(project, endpoint);
- if (odahuDTO.isPresent() && UserInstanceStatus.RUNNING ==
odahuDTO.get().getStatus()) {
- odahuDAO.updateStatus(name, project, endpoint,
UserInstanceStatus.TERMINATING);
- actionOnCloud(user, TERMINATE_ODAHU_API, name, project, endpoint);
- } else {
- log.error("Cannot terminate odahu cluster {}", odahuDTO);
- throw new DlabException(String.format("Cannot terminate odahu
cluster %s", odahuDTO));
- }
+ List<OdahuDTO> odahuDTOS = findOdahu();
+ odahuDTOS.stream()
Review comment:
Move **.stream()** to new line
##########
File path:
services/self-service/src/main/java/com/epam/dlab/backendapi/service/impl/OdahuServiceImpl.java
##########
@@ -132,14 +134,20 @@ public void stop(String name, String project, String
endpoint, UserInfo user) {
@Override
public void terminate(String name, String project, String endpoint,
UserInfo user) {
- Optional<OdahuDTO> odahuDTO = get(project, endpoint);
- if (odahuDTO.isPresent() && UserInstanceStatus.RUNNING ==
odahuDTO.get().getStatus()) {
- odahuDAO.updateStatus(name, project, endpoint,
UserInstanceStatus.TERMINATING);
- actionOnCloud(user, TERMINATE_ODAHU_API, name, project, endpoint);
- } else {
- log.error("Cannot terminate odahu cluster {}", odahuDTO);
- throw new DlabException(String.format("Cannot terminate odahu
cluster %s", odahuDTO));
- }
+ List<OdahuDTO> odahuDTOS = findOdahu();
Review comment:
Can we use the method which returns not all Odahu clusters, but only per
**name** and per ** project**?
##########
File path:
services/self-service/src/main/java/com/epam/dlab/backendapi/service/impl/OdahuServiceImpl.java
##########
@@ -91,7 +91,9 @@ public OdahuServiceImpl(ProjectService projectService,
EndpointService endpointS
@BudgetLimited
@Override
public void create(@Project String project, OdahuCreateDTO odahuCreateDTO,
UserInfo user) {
- Optional<OdahuDTO> odahuDTO =
odahuDAO.getByProjectEndpoint(odahuCreateDTO.getProject(),
odahuCreateDTO.getEndpoint());
+ Optional<OdahuDTO> odahuDTO = odahuDAO
+ .getByProjectEndpoint(odahuCreateDTO.getProject(),
odahuCreateDTO.getEndpoint())
+ .filter(p -> p.getStatus().equals(UserInstanceStatus.FAILED));
Review comment:
Is it equals to **FAILED** or **not** equals?
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
