[GitHub] [submarine] pingsutw commented on a change in pull request #655: SUBMARINE-851. Dirty notebook status is persisted in DB when notebook operation is failed

[GitBox]

pingsutw commented on a change in pull request #655:
URL: https://github.com/apache/submarine/pull/655#discussion_r665841002



##########
File path: 
submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sSubmitter.java
##########
@@ -361,42 +361,57 @@ public Notebook createNotebook(NotebookSpec spec) throws 
SubmarineRuntimeExcepti
     final String storage = NotebookUtils.STORAGE;
     final String pvcName = NotebookUtils.PVC_PREFIX + name;
     String namespace = "default";
-    
+
     if (System.getenv(ENV_NAMESPACE) != null) {
       namespace = System.getenv(ENV_NAMESPACE);
     }
-    
+
+    // create notebook custom resource
+    NotebookCR notebookCR = NotebookSpecParser.parseNotebook(spec);
+    Map<String, String> labels = new HashMap<>();
+    labels.put(NotebookCR.NOTEBOOK_OWNER_SELECTOR_KET, 
spec.getMeta().getOwnerId());
+    notebookCR.getMetadata().setLabels(labels);
+    notebookCR.getMetadata().setNamespace(namespace);
+
+    // create persistent volume
     try {
-      // create notebook custom resource
-      NotebookCR notebookCR = NotebookSpecParser.parseNotebook(spec);
-      Map<String, String> labels = new HashMap<>();
-      labels.put(NotebookCR.NOTEBOOK_OWNER_SELECTOR_KET, 
spec.getMeta().getOwnerId());
-      notebookCR.getMetadata().setLabels(labels);
-      notebookCR.getMetadata().setNamespace(namespace);
-      
-      // create persistent volume
       createPersistentVolume(pvName, host, storage);
+    } catch (ApiException e) {
+      LOG.error("K8s submitter: parse Notebook object failed by " + 
e.getMessage(), e);
+      throw new SubmarineRuntimeException(e.getCode(), "K8s submitter: parse 
Notebook object failed by " +
+          e.getMessage());
+    }
 
-      // create persistent volume claim
+    // create persistent volume claim
+    try {
       createPersistentVolumeClaim(pvcName, namespace, pvName, storage);
+    } catch (ApiException e) {
+      LOG.error("K8s submitter: parse Notebook object failed by " + 
e.getMessage(), e);

Review comment:
       ```suggestion
         LOG.error("K8s submitter: Create persistent volumn claim for Notebook 
object failed by " + e.getMessage(), e);
   ```

##########
File path: 
submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sSubmitter.java
##########
@@ -361,42 +361,57 @@ public Notebook createNotebook(NotebookSpec spec) throws 
SubmarineRuntimeExcepti
     final String storage = NotebookUtils.STORAGE;
     final String pvcName = NotebookUtils.PVC_PREFIX + name;
     String namespace = "default";
-    
+
     if (System.getenv(ENV_NAMESPACE) != null) {
       namespace = System.getenv(ENV_NAMESPACE);
     }
-    
+
+    // create notebook custom resource
+    NotebookCR notebookCR = NotebookSpecParser.parseNotebook(spec);
+    Map<String, String> labels = new HashMap<>();
+    labels.put(NotebookCR.NOTEBOOK_OWNER_SELECTOR_KET, 
spec.getMeta().getOwnerId());
+    notebookCR.getMetadata().setLabels(labels);
+    notebookCR.getMetadata().setNamespace(namespace);

Review comment:
       We should put these codes to try-block to prevent parsing Notebook 
object failed.

##########
File path: 
submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sSubmitter.java
##########
@@ -361,42 +361,57 @@ public Notebook createNotebook(NotebookSpec spec) throws 
SubmarineRuntimeExcepti
     final String storage = NotebookUtils.STORAGE;
     final String pvcName = NotebookUtils.PVC_PREFIX + name;
     String namespace = "default";
-    
+
     if (System.getenv(ENV_NAMESPACE) != null) {
       namespace = System.getenv(ENV_NAMESPACE);
     }
-    
+
+    // create notebook custom resource
+    NotebookCR notebookCR = NotebookSpecParser.parseNotebook(spec);
+    Map<String, String> labels = new HashMap<>();
+    labels.put(NotebookCR.NOTEBOOK_OWNER_SELECTOR_KET, 
spec.getMeta().getOwnerId());
+    notebookCR.getMetadata().setLabels(labels);
+    notebookCR.getMetadata().setNamespace(namespace);
+
+    // create persistent volume
     try {
-      // create notebook custom resource
-      NotebookCR notebookCR = NotebookSpecParser.parseNotebook(spec);
-      Map<String, String> labels = new HashMap<>();
-      labels.put(NotebookCR.NOTEBOOK_OWNER_SELECTOR_KET, 
spec.getMeta().getOwnerId());
-      notebookCR.getMetadata().setLabels(labels);
-      notebookCR.getMetadata().setNamespace(namespace);
-      
-      // create persistent volume
       createPersistentVolume(pvName, host, storage);
+    } catch (ApiException e) {
+      LOG.error("K8s submitter: parse Notebook object failed by " + 
e.getMessage(), e);
+      throw new SubmarineRuntimeException(e.getCode(), "K8s submitter: parse 
Notebook object failed by " +
+          e.getMessage());
+    }
 
-      // create persistent volume claim
+    // create persistent volume claim
+    try {
       createPersistentVolumeClaim(pvcName, namespace, pvName, storage);
+    } catch (ApiException e) {
+      LOG.error("K8s submitter: parse Notebook object failed by " + 
e.getMessage(), e);
+      rollbackCreationNotebook(pvName);
+      throw new SubmarineRuntimeException(e.getCode(), "K8s submitter: parse 
Notebook object failed by " +

Review comment:
       ditto

##########
File path: 
submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sSubmitter.java
##########
@@ -361,42 +361,57 @@ public Notebook createNotebook(NotebookSpec spec) throws 
SubmarineRuntimeExcepti
     final String storage = NotebookUtils.STORAGE;
     final String pvcName = NotebookUtils.PVC_PREFIX + name;
     String namespace = "default";
-    
+
     if (System.getenv(ENV_NAMESPACE) != null) {
       namespace = System.getenv(ENV_NAMESPACE);
     }
-    
+
+    // create notebook custom resource
+    NotebookCR notebookCR = NotebookSpecParser.parseNotebook(spec);
+    Map<String, String> labels = new HashMap<>();
+    labels.put(NotebookCR.NOTEBOOK_OWNER_SELECTOR_KET, 
spec.getMeta().getOwnerId());
+    notebookCR.getMetadata().setLabels(labels);
+    notebookCR.getMetadata().setNamespace(namespace);
+
+    // create persistent volume
     try {
-      // create notebook custom resource
-      NotebookCR notebookCR = NotebookSpecParser.parseNotebook(spec);
-      Map<String, String> labels = new HashMap<>();
-      labels.put(NotebookCR.NOTEBOOK_OWNER_SELECTOR_KET, 
spec.getMeta().getOwnerId());
-      notebookCR.getMetadata().setLabels(labels);
-      notebookCR.getMetadata().setNamespace(namespace);
-      
-      // create persistent volume
       createPersistentVolume(pvName, host, storage);
+    } catch (ApiException e) {
+      LOG.error("K8s submitter: parse Notebook object failed by " + 
e.getMessage(), e);

Review comment:
       ```suggestion
         LOG.error("K8s submitter: Create persistent volume for Notebook object 
failed by " + e.getMessage(), e);
   ```




-- 
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.

To unsubscribe, e-mail: commits-unsubscr...@submarine.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org