jarek/7562: fix heap size fp problem (#7564)

* #7562: fix heap size fp problem * #7562: fix version and heap size fp problem * #7562 fix jvm settings overwriting whole beakerx config file * #7562 fix error on beakerx.json not exist * cleanup formatting of heap size as string * clean up floating point handling of heap size
twosigma · Jun 21, 2018 · 6bcfd14 · 6bcfd14
1 parent 34d39e0
commit 6bcfd14
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 21 deletions.
diff --git a/beakerx/beakerx/environment.py b/beakerx/beakerx/environment.py
@@ -50,8 +50,18 @@ def __init__(self):
     @staticmethod
     def save_setting_to_file(content):
         makedirs(paths.jupyter_config_dir(), exist_ok=True)
-        with fdopen(osopen(EnvironmentSettings.config_path, O_RDWR | O_CREAT | O_TRUNC, 0o600), 'w+') as file:
-            file.write(json.dumps(json.loads(content), indent=4, sort_keys=True))
+        with fdopen(osopen(EnvironmentSettings.config_path, O_RDWR | O_CREAT, 0o600), 'w+') as file:
+            file_content = file.read()
+            new_settings = json.loads(content)
+            if file_content:
+                saved_settings = json.loads(file_content)
+                file.seek(0)
+                file.truncate()
+                for setting_name in new_settings['beakerx']:
+                    saved_settings['beakerx'][setting_name] = new_settings['beakerx'][setting_name]
+            else:
+                saved_settings = new_settings
+            file.write(json.dumps(saved_settings, indent=4, sort_keys=True))
 
     @staticmethod
     def read_setting_from_file():
@@ -108,7 +118,7 @@ def read_beakerx_env_settings():
             if 'heap_GB' in jvm_settings and jvm_settings['heap_GB']:
                 val = float(jvm_settings['heap_GB'])
                 if val.is_integer():
-                    value = '-Xmx' + str(jvm_settings['heap_GB']) + 'g'
+                    value = '-Xmx' + str(int(val)) + 'g'
                 else:
                     value = '-Xmx' + str(int(val * 1024)) + 'm'
                 args.append(value)

diff --git a/js/notebook/src/tree/Models/DefaultOptionsModel.ts b/js/notebook/src/tree/Models/DefaultOptionsModel.ts
@@ -27,9 +27,9 @@ export default class DefaultOptionsModel {
   }
 
   public static normaliseHeapSize(heap_GB: number): string {
-    if (heap_GB % 1 === 0) {
-      return `${heap_GB}g`;
+    if (Number.isInteger(heap_GB)) {
+      return heap_GB + 'g';
     }
-    return parseInt(`${heap_GB * 1024}`) + 'm';
+    return Math.floor(heap_GB * 1024) + 'm';
   }
 }
diff --git a/kernel/sparkex/src/main/java/com/twosigma/beakerx/widget/SparkUIForm.java b/kernel/sparkex/src/main/java/com/twosigma/beakerx/widget/SparkUIForm.java
@@ -151,7 +151,7 @@ private void loadProfile() {
       this.executorCores.setValue(profileData.getOrDefault(SparkUI.SPARK_EXECUTOR_CORES, SparkUI.SPARK_EXECUTOR_CORES_DEFAULT));
       this.executorMemory.setValue(profileData.getOrDefault(SparkUI.SPARK_EXECUTOR_MEMORY, SparkUI.SPARK_EXECUTOR_MEMORY_DEFAULT));
       Map<String, String> advancedSettings = new HashMap<>();
-      ((ArrayList<LinkedTreeMap>)profileData.getOrDefault(SparkUI.SPARK_ADVANCED_OPTIONS, SparkUI.SPARK_ADVANCED_OPTIONS_DEFAULT))
+      ((ArrayList<Map>)profileData.getOrDefault(SparkUI.SPARK_ADVANCED_OPTIONS, SparkUI.SPARK_ADVANCED_OPTIONS_DEFAULT))
               .stream()
               .forEach(x -> {
                 advancedSettings.put(x.get("name").toString(), x.get("value").toString());

diff --git a/kernel/sparkex/src/main/java/com/twosigma/beakerx/widget/SparkUiDefaultsImpl.java b/kernel/sparkex/src/main/java/com/twosigma/beakerx/widget/SparkUiDefaultsImpl.java
@@ -15,9 +15,8 @@
  */
 package com.twosigma.beakerx.widget;
 
-import com.google.gson.Gson;
-import com.google.gson.GsonBuilder;
-import com.google.gson.internal.LinkedTreeMap;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializationFeature;
 import org.apache.spark.SparkConf;
 import org.apache.spark.sql.SparkSession;
 import scala.Tuple2;
@@ -29,15 +28,13 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
-import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 
 import static com.twosigma.beakerx.widget.SparkUI.BEAKERX_ID;
-import static com.twosigma.beakerx.widget.SparkUI.SPARK_ADVANCED_OPTIONS_DEFAULT;
 import static com.twosigma.beakerx.widget.SparkUI.SPARK_APP_NAME;
 import static com.twosigma.beakerx.widget.SparkUI.SPARK_EXECUTOR_CORES_DEFAULT;
 import static com.twosigma.beakerx.widget.SparkUI.SPARK_EXECUTOR_MEMORY_DEFAULT;
@@ -59,12 +56,14 @@ public class SparkUiDefaultsImpl implements SparkUiDefaults {
   private static final String CURRENT_PROFILE = "current_profile";
 
   private List<Map<String, Object>> profiles = new ArrayList<>();
-  private Gson gson = new GsonBuilder().setPrettyPrinting().create();
+  private ObjectMapper objectMapper;
   private Path path;
   private String currentProfile = DEFAULT_PROFILE;
 
   public SparkUiDefaultsImpl(Path path) {
     this.path = path;
+    this.objectMapper = new ObjectMapper();
+    this.objectMapper.enable(SerializationFeature.INDENT_OUTPUT);
   }
 
   public void saveSparkConf(List<Map<String, Object>> profiles) {
@@ -73,7 +72,7 @@ public void saveSparkConf(List<Map<String, Object>> profiles) {
       Map<String, Object> sparkOptions = (Map<String, Object>) map.get(BEAKERX).getOrDefault(SPARK_OPTIONS, new HashMap<>());
       sparkOptions.put(SPARK_PROFILES, profiles == null ? new ArrayList<>() : profiles);
       map.get(BEAKERX).put(SPARK_OPTIONS, sparkOptions);
-      String content = gson.toJson(map);
+      String content = toJson(map);
       Files.write(path, content.getBytes(StandardCharsets.UTF_8));
       this.profiles = profiles;
     } catch (IOException e) {
@@ -145,11 +144,12 @@ public List<String> getProfileNames() {
   @Override
   public void saveProfileName(String profileName) {
     try {
+
       Map<String, Map> map = beakerxJsonAsMap(path);
       Map<String, Object> sparkOptions = (Map<String, Object>) map.get(BEAKERX).getOrDefault(SPARK_OPTIONS, new HashMap<>());
       sparkOptions.put(CURRENT_PROFILE, profileName);
       map.get(BEAKERX).put(SPARK_OPTIONS, sparkOptions);
-      String content = gson.toJson(map);
+      String content = toJson(map);
       Files.write(path, content.getBytes(StandardCharsets.UTF_8));
       currentProfile = profileName;
     } catch (IOException e) {
@@ -195,7 +195,7 @@ Map<String, Map> beakerxJsonAsMap(Path path) {
     } catch (Exception e) {
       throw new RuntimeException(e);
     }
-    return (Map<String, Map>) gson.fromJson(jsonAsString, Map.class);
+    return fromJson(jsonAsString, LinkedHashMap.class);
   }
 
   private Map<String, Object> toMap(SparkConf sparkConf) {
@@ -231,4 +231,23 @@ private List<Map<String, String>> getProps(Map<String, Object> result) {
     return (List<Map<String, String>>) propsAsObject;
   }
 
+  private <T> T fromJson(String json, Class<T> theClass) {
+    T result = null;
+    try {
+      result = objectMapper.readValue(json, theClass);
+    } catch (Exception e) {
+      // Ignored.
+    }
+
+    return result;
+  }
+
+  private String toJson(Object object) {
+    try {
+      return objectMapper.writeValueAsString(object);
+    } catch (Exception e) {
+      return null;
+    }
+  }
+
 }
diff --git a/kernel/sparkex/src/test/java/com/twosigma/beakerx/widget/SparkUiDefaultsImplTest.java b/kernel/sparkex/src/test/java/com/twosigma/beakerx/widget/SparkUiDefaultsImplTest.java
@@ -18,9 +18,11 @@
 
 import org.apache.spark.SparkConf;
 import org.apache.spark.sql.SparkSession;
+import org.assertj.core.api.Assertions;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.io.IOException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
@@ -36,7 +38,6 @@
 import static com.twosigma.beakerx.widget.SparkUiDefaults.DEFAULT_PROFILE;
 import static com.twosigma.beakerx.widget.SparkUiDefaultsImpl.BEAKERX;
 import static com.twosigma.beakerx.widget.SparkUiDefaultsImpl.PROPERTIES;
-import static com.twosigma.beakerx.widget.SparkUiDefaultsImpl.SPARK_OPTIONS;
 import static com.twosigma.beakerx.widget.SparkUiDefaultsImpl.VALUE;
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -56,6 +57,20 @@ public void setUp() {
     this.sut = new SparkUiDefaultsImpl(pathToBeakerxTestJson);
   }
 
+  @Test
+  public void loadAndSaveWithoutChangesShouldBeIdempotent() throws IOException {
+    //given
+    SparkSession.Builder builder = SparkSession.builder();
+    HashMap<String, Object> profileConfig = new HashMap<>();
+    profileConfig.put(NAME, DEFAULT_PROFILE);
+    //when
+    sut.loadDefaults(builder);
+    sut.saveProfile(profileConfig);
+    //then
+    Map<String, Map> result = sut.beakerxJsonAsMap(pathToBeakerxTestJson);
+    Assertions.assertThat(result.get("beakerx").get("version")).isEqualTo(2);
+  }
+
   @Test
   public void saveMasterURL() {
     //given
@@ -137,7 +152,7 @@ public void saveAndLoadDefaults() {
     //given
     HashMap<String, Object> profileConfig = new HashMap<>();
     profileConfig.put(SPARK_ADVANCED_OPTIONS, Arrays.asList(
-            new SparkConfiguration.Configuration("sparkOption2", "sp2")));
+            new SparkConfiguration.Configuration("sparkOption2", "3")));
     profileConfig.put(SPARK_MASTER, "local[4]");
     profileConfig.put(NAME, DEFAULT_PROFILE);
 
@@ -149,7 +164,7 @@ public void saveAndLoadDefaults() {
     SparkSession.Builder builder = SparkSession.builder();
     sut.loadDefaults(builder);
     SparkConf sparkConfBasedOn = SparkEngineImpl.getSparkConfBasedOn(builder);
-    assertThat(sparkConfBasedOn.get("sparkOption2")).isEqualTo("sp2");
+    assertThat(sparkConfBasedOn.get("sparkOption2")).isEqualTo("3");
     assertThat(sparkConfBasedOn.get(SPARK_MASTER)).isEqualTo("local[4]");
   }
 

diff --git a/kernel/sparkex/src/test/resources/beakerxTest.json b/kernel/sparkex/src/test/resources/beakerxTest.json
@@ -8,6 +8,6 @@
     "ui_options": {
       "auto_close": true
     },
-    "version": 2.0
+    "version": 2
   }
 }