Repository: zeppelin Updated Branches: refs/heads/master 99b90effd -> ac1e73c50
[ZEPPELIN-2103] Unnecessary read to Helium registry ### What is this PR for? Every `Helium.getAllPackageInfo()` call read helium package information from all registry configured (both local registry, online registry by default). Problem is, `Helium.getAllPackageInfo()` is used inside of many other methods. like `Helium.suggestApp()`, `Helium.enable()`, `Helium.disable()`, `Helium.recreateBundle()`, `Helium.getPackageInfo()`. So local/remote registry is unnecessarily accessed more than it should do. ### What type of PR is it? Bug Fix ### Todos * [x] - Hold the result and reuse it * [x] - Unit test ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-2103 ### How should this be tested? Unittest included ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: Lee moon soo <[email protected]> Closes #2015 from Leemoonsoo/ZEPPELIN-2103 and squashes the following commits: 57d19f7 [Lee moon soo] Hold package info and reuse unless refresh flag set to true Project: http://git-wip-us.apache.org/repos/asf/zeppelin/repo Commit: http://git-wip-us.apache.org/repos/asf/zeppelin/commit/ac1e73c5 Tree: http://git-wip-us.apache.org/repos/asf/zeppelin/tree/ac1e73c5 Diff: http://git-wip-us.apache.org/repos/asf/zeppelin/diff/ac1e73c5 Branch: refs/heads/master Commit: ac1e73c5053c3c764cd3e82047368b1988d9d911 Parents: 99b90ef Author: Lee moon soo <[email protected]> Authored: Mon Feb 13 19:59:30 2017 +0900 Committer: Lee moon soo <[email protected]> Committed: Thu Feb 23 11:31:14 2017 +0900 ---------------------------------------------------------------------- .../java/org/apache/zeppelin/helium/Helium.java | 79 +++++++++++++------- .../org/apache/zeppelin/helium/HeliumTest.java | 39 ++++++++++ 2 files changed, 90 insertions(+), 28 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/zeppelin/blob/ac1e73c5/zeppelin-zengine/src/main/java/org/apache/zeppelin/helium/Helium.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/helium/Helium.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/helium/Helium.java index e2e1b49..918e9aa 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/helium/Helium.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/helium/Helium.java @@ -48,6 +48,8 @@ public class Helium { private final HeliumBundleFactory bundleFactory; private final HeliumApplicationFactory applicationFactory; + Map<String, List<HeliumPackageSearchResult>> allPackages; + public Helium( String heliumConfPath, String registryPaths, @@ -142,7 +144,7 @@ public class Helium { } private void clearNotExistsPackages() { - Map<String, List<HeliumPackageSearchResult>> all = getAllPackageInfo(); + Map<String, List<HeliumPackageSearchResult>> all = getAllPackageInfo(false); // clear visualization display order List<String> packageOrder = heliumConf.getBundleDisplayOrder(); @@ -164,43 +166,64 @@ public class Helium { } public Map<String, List<HeliumPackageSearchResult>> getAllPackageInfo() { + return getAllPackageInfo(true); + } + + public Map<String, List<HeliumPackageSearchResult>> getAllPackageInfo(boolean refresh) { Map<String, String> enabledPackageInfo = heliumConf.getEnabledPackages(); - Map<String, List<HeliumPackageSearchResult>> map = new HashMap<>(); synchronized (registry) { - for (HeliumRegistry r : registry) { - try { - for (HeliumPackage pkg : r.getAll()) { - String name = pkg.getName(); - String artifact = enabledPackageInfo.get(name); - boolean enabled = (artifact != null && artifact.equals(pkg.getArtifact())); - - if (!map.containsKey(name)) { - map.put(name, new LinkedList<HeliumPackageSearchResult>()); + if (refresh || allPackages == null) { + allPackages = new HashMap<>(); + for (HeliumRegistry r : registry) { + try { + for (HeliumPackage pkg : r.getAll()) { + String name = pkg.getName(); + String artifact = enabledPackageInfo.get(name); + boolean enabled = (artifact != null && artifact.equals(pkg.getArtifact())); + + if (!allPackages.containsKey(name)) { + allPackages.put(name, new LinkedList<HeliumPackageSearchResult>()); + } + allPackages.get(name).add(new HeliumPackageSearchResult(r.name(), pkg, enabled)); } - map.get(name).add(new HeliumPackageSearchResult(r.name(), pkg, enabled)); + } catch (IOException e) { + logger.error(e.getMessage(), e); } - } catch (IOException e) { - logger.error(e.getMessage(), e); } - } - } + } else { - // sort version (artifact) - for (String name : map.keySet()) { - List<HeliumPackageSearchResult> packages = map.get(name); - Collections.sort(packages, new Comparator<HeliumPackageSearchResult>() { - @Override - public int compare(HeliumPackageSearchResult o1, HeliumPackageSearchResult o2) { - return o2.getPkg().getArtifact().compareTo(o1.getPkg().getArtifact()); + for (String name : allPackages.keySet()) { + List<HeliumPackageSearchResult> pkgs = allPackages.get(name); + String artifact = enabledPackageInfo.get(name); + LinkedList<HeliumPackageSearchResult> newResults = + new LinkedList<HeliumPackageSearchResult>(); + + for (HeliumPackageSearchResult pkg : pkgs) { + boolean enabled = (artifact != null && artifact.equals(pkg.getPkg().getArtifact())); + newResults.add(new HeliumPackageSearchResult(pkg.getRegistry(), pkg.getPkg(), enabled)); + } + + allPackages.put(name, newResults); } - }); + } + + // sort version (artifact) + for (String name : allPackages.keySet()) { + List<HeliumPackageSearchResult> packages = allPackages.get(name); + Collections.sort(packages, new Comparator<HeliumPackageSearchResult>() { + @Override + public int compare(HeliumPackageSearchResult o1, HeliumPackageSearchResult o2) { + return o2.getPkg().getArtifact().compareTo(o1.getPkg().getArtifact()); + } + }); + } + return allPackages; } - return map; } public HeliumPackageSearchResult getPackageInfo(String name, String artifact) { - Map<String, List<HeliumPackageSearchResult>> infos = getAllPackageInfo(); + Map<String, List<HeliumPackageSearchResult>> infos = getAllPackageInfo(false); List<HeliumPackageSearchResult> packages = infos.get(name); if (artifact == null) { return packages.get(0); @@ -276,7 +299,7 @@ public class Helium { allResources = ResourcePoolUtils.getAllResources(); } - for (List<HeliumPackageSearchResult> pkgs : getAllPackageInfo().values()) { + for (List<HeliumPackageSearchResult> pkgs : getAllPackageInfo(false).values()) { for (HeliumPackageSearchResult pkg : pkgs) { if (pkg.getPkg().getType() == HeliumType.APPLICATION && pkg.isEnabled()) { ResourceSet resources = ApplicationLoader.findRequiredResourceSet( @@ -304,7 +327,7 @@ public class Helium { * @return ordered list of enabled buildBundle package */ public List<HeliumPackage> getBundlePackagesToBundle() { - Map<String, List<HeliumPackageSearchResult>> allPackages = getAllPackageInfo(); + Map<String, List<HeliumPackageSearchResult>> allPackages = getAllPackageInfo(false); List<String> visOrder = heliumConf.getBundleDisplayOrder(); List<HeliumPackage> orderedBundlePackages = new LinkedList<>(); http://git-wip-us.apache.org/repos/asf/zeppelin/blob/ac1e73c5/zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumTest.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumTest.java index 1607c2c..9301c18 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumTest.java +++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumTest.java @@ -100,4 +100,43 @@ public class HeliumTest { // then assertEquals(2, helium.getAllPackageInfo().size()); } + + + @Test + public void testRefresh() throws IOException, URISyntaxException, TaskRunnerException { + File heliumConf = new File(tmpDir, "helium.conf"); + Helium helium = new Helium( + heliumConf.getAbsolutePath(), localRegistryPath.getAbsolutePath(), null, null, null); + HeliumTestRegistry registry1 = new HeliumTestRegistry("r1", "r1"); + helium.addRegistry(registry1); + + // when + registry1.add(new HeliumPackage( + HeliumType.APPLICATION, + "name1", + "desc1", + "artifact1", + "className1", + new String[][]{}, + "", + "")); + + // then + assertEquals(1, helium.getAllPackageInfo(false).size()); + + // when + registry1.add(new HeliumPackage( + HeliumType.APPLICATION, + "name2", + "desc2", + "artifact2", + "className2", + new String[][]{}, + "", + "")); + + // then + assertEquals(1, helium.getAllPackageInfo(false).size()); + assertEquals(2, helium.getAllPackageInfo(true).size()); + } }
