[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15153741#comment-15153741
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/374


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15149344#comment-15149344
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on the pull request:

https://github.com/apache/drill/pull/374#issuecomment-184883435
  
I'm +1 on my patch assuming the manually testing that Sudheesh is asking 
about.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15149111#comment-15149111
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on the pull request:

https://github.com/apache/drill/pull/374#issuecomment-184833965
  
Since there are no unit tests for web UI, can you do some manual testing to 
ensure there are no regressions?

Is there a simple way to test all persistent and transient stores? I see 
tests for a few implementations only.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15149101#comment-15149101
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r53059667
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/CachingPersistentStoreProvider.java
 ---
@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys.store.provider;
+
+import java.util.List;
+import java.util.concurrent.ConcurrentMap;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import org.apache.drill.common.AutoCloseables;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.PersistentStoreProvider;
+
+public class CachingPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(CachingPersistentStoreProvider.class);
+
+  private final ConcurrentMap storeCache = Maps.newConcurrentMap();
+  private final PersistentStoreProvider provider;
+
+  public CachingPersistentStoreProvider(PersistentStoreProvider provider) {
+this.provider = provider;
+  }
+
+  @SuppressWarnings("unchecked")
+  public  PersistentStore getOrCreateStore(final 
PersistentStoreConfig config) throws StoreException {
--- End diff --

override


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148017#comment-15148017
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52962267
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/LocalPersistentStoreProvider.java
 ---
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys.store.provider;
+
+import java.io.IOException;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.dfs.DrillFileSystem;
+import org.apache.drill.exec.store.sys.PersistentStoreRegistry;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.store.LocalPersistentStore;
+import org.apache.drill.exec.testing.store.NoWriteLocalStore;
+import org.apache.hadoop.fs.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A really simple provider that stores data in the local file system, one 
value per file.
+ */
+public class LocalPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final Logger logger = 
LoggerFactory.getLogger(LocalPersistentStoreProvider.class);
+
+  private final Path path;
+  private final DrillFileSystem fs;
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148015#comment-15148015
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52962203
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/CachingPersistentStoreProvider.java
 ---
@@ -0,0 +1,72 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys.store.provider;
+
+import java.util.concurrent.ConcurrentMap;
+
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.PersistentStoreProvider;
+
+public class CachingPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(CachingPersistentStoreProvider.class);
+
+  private final ConcurrentMap storeCache = Maps.newConcurrentMap();
+  private final PersistentStoreProvider provider;
+
+  public CachingPersistentStoreProvider(PersistentStoreProvider provider) {
+this.provider = provider;
+  }
+
+  @SuppressWarnings("unchecked")
+  public  PersistentStore getOrCreateStore(final 
PersistentStoreConfig config) throws StoreException {
+final PersistentStore store = storeCache.get(config);
+if (store == null) {
+  final PersistentStore newStore = 
provider.getOrCreateStore(config);
+  final PersistentStore finalStore = storeCache.putIfAbsent(config, 
newStore);
+  if (finalStore == null) {
+return (PersistentStore)newStore;
+  }
+  try {
+newStore.close();
+  } catch (Exception ex) {
+throw new StoreException(ex);
+  }
+}
+
+return (PersistentStore) store;
+
+  }
+
+  @Override
+  public void start() throws Exception {
+provider.start();
+  }
+
+  @Override
+  public void close() throws Exception {
+for(final PersistentStore store : storeCache.values()){
+  store.close();
--- End diff --

AutoCloseables.close(storecache.values() + provider). Done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148013#comment-15148013
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52962000
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -146,38 +154,37 @@ public QProfiles(List runningQueries, 
List finishedQue
   @Path("/profiles.json")
   @Produces(MediaType.APPLICATION_JSON)
   public QProfiles getProfilesJSON() {
-PStore completed = null;
-PStore running = null;
-try {
-  completed = provider().getStore(QueryManager.QUERY_PROFILE);
-  running = provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-} catch (IOException e) {
-  logger.debug("Failed to get profiles from persistent or ephemeral 
store.");
-  return new QProfiles(new ArrayList(), new 
ArrayList());
-}
-
-List runningQueries = Lists.newArrayList();
-
-for (Map.Entry entry : running) {
-  QueryInfo profile = entry.getValue();
-  if (principal.canManageProfileOf(profile.getUser())) {
-runningQueries.add(new ProfileInfo(entry.getKey(), 
profile.getStart(), profile.getForeman().getAddress(),
-profile.getQuery(), profile.getState().name(), 
profile.getUser()));
+try (
+final PersistentStore completed = 
getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE);
--- End diff --

Fixed.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147998#comment-15147998
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52961092
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/LocalPersistentStoreProvider.java
 ---
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys.store.provider;
+
+import java.io.IOException;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.dfs.DrillFileSystem;
+import org.apache.drill.exec.store.sys.PersistentStoreRegistry;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.store.LocalPersistentStore;
+import org.apache.drill.exec.testing.store.NoWriteLocalStore;
+import org.apache.hadoop.fs.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A really simple provider that stores data in the local file system, one 
value per file.
+ */
+public class LocalPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final Logger logger = 
LoggerFactory.getLogger(LocalPersistentStoreProvider.class);
+
+  private final Path path;
+  private final DrillFileSystem fs;
--- End diff --

Shouldn't fs be closed in close()?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148002#comment-15148002
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52961125
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/ZookeeperPersistentStoreProvider.java
 ---
@@ -0,0 +1,85 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys.store.provider;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.coord.zk.ZKClusterCoordinator;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.dfs.DrillFileSystem;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreRegistry;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.store.LocalPersistentStore;
+import org.apache.drill.exec.store.sys.store.ZookeeperPersistentStore;
+import org.apache.hadoop.fs.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZookeeperPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final Logger logger = 
LoggerFactory.getLogger(ZookeeperPersistentStoreProvider.class);
+
+  private static final String DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT = 
"drill.exec.sys.store.provider.zk.blobroot";
+
+  private final CuratorFramework curator;
+  private final DrillFileSystem fs;
--- End diff --

Shouldn't fs be closed in close()?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147988#comment-15147988
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52960719
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/CachingPersistentStoreProvider.java
 ---
@@ -0,0 +1,72 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys.store.provider;
+
+import java.util.concurrent.ConcurrentMap;
+
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.PersistentStoreProvider;
+
+public class CachingPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(CachingPersistentStoreProvider.class);
+
+  private final ConcurrentMap storeCache = Maps.newConcurrentMap();
+  private final PersistentStoreProvider provider;
+
+  public CachingPersistentStoreProvider(PersistentStoreProvider provider) {
+this.provider = provider;
+  }
+
+  @SuppressWarnings("unchecked")
+  public  PersistentStore getOrCreateStore(final 
PersistentStoreConfig config) throws StoreException {
+final PersistentStore store = storeCache.get(config);
+if (store == null) {
+  final PersistentStore newStore = 
provider.getOrCreateStore(config);
+  final PersistentStore finalStore = storeCache.putIfAbsent(config, 
newStore);
+  if (finalStore == null) {
+return (PersistentStore)newStore;
+  }
+  try {
+newStore.close();
+  } catch (Exception ex) {
+throw new StoreException(ex);
+  }
+}
+
+return (PersistentStore) store;
+
+  }
+
+  @Override
+  public void start() throws Exception {
+provider.start();
+  }
+
+  @Override
+  public void close() throws Exception {
+for(final PersistentStore store : storeCache.values()){
+  store.close();
--- End diff --

AutoCloseables.close(storeCache.values());


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147989#comment-15147989
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52960723
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/ZookeeperPersistentStoreProvider.java
 ---
@@ -0,0 +1,85 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys.store.provider;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.coord.zk.ZKClusterCoordinator;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.dfs.DrillFileSystem;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreRegistry;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.store.LocalPersistentStore;
+import org.apache.drill.exec.store.sys.store.ZookeeperPersistentStore;
+import org.apache.hadoop.fs.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZookeeperPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final Logger logger = 
LoggerFactory.getLogger(ZookeeperPersistentStoreProvider.class);
+
+  private static final String DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT = 
"drill.exec.sys.store.provider.zk.blobroot";
+
+  private final CuratorFramework curator;
+  private final DrillFileSystem fs;
+  private final Path blobRoot;
+
+  public ZookeeperPersistentStoreProvider(final 
PersistentStoreRegistry registry) throws StoreException {
+this(registry.getConfig(), registry.getCoordinator().getCurator());
+  }
+
+  @VisibleForTesting
+  public ZookeeperPersistentStoreProvider(final DrillConfig config, final 
CuratorFramework curator) throws StoreException {
+this.curator = curator;
+
+if (config.hasPath(DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT)) {
+  blobRoot = new 
Path(config.getString(DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT));
+}else{
+  blobRoot = LocalPersistentStore.getLogDir();
+}
+
+try {
+  this.fs = LocalPersistentStore.getFileSystem(config, blobRoot);
+} catch (IOException ex) {
+  throw new StoreException("unable to get filesystem", ex);
+}
+  }
+
+  @Override
+  public  PersistentStore getOrCreateStore(final 
PersistentStoreConfig config) throws StoreException {
+switch(config.getMode()){
+case BLOB_PERSISTENT:
+  return new LocalPersistentStore<>(fs, blobRoot, config);
--- End diff --

LocalPersistentStore in ZookeeperPersistentStoreProvider?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store 

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147981#comment-15147981
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52960456
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -217,11 +223,14 @@ private QueryProfile getQueryProfile(String queryId) 
throws IOException {
 }
 
 // then check blob store
-PStore profiles = 
provider().getStore(QueryManager.QUERY_PROFILE);
-QueryProfile queryProfile = profiles.get(queryId);
-if (queryProfile != null) {
-  checkOrThrowProfileViewAuthorization(queryProfile);
-  return queryProfile;
+try (final PersistentStore profiles = 
getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE)) {
--- End diff --

same here


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147980#comment-15147980
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52960453
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -200,9 +207,8 @@ private QueryProfile getQueryProfile(String queryId) 
throws IOException {
 }
 
 // then check remote running
-try{
-  PStore runningQueries = 
provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-  QueryInfo info = runningQueries.get(queryId);
+try (final TransientStore running = 
getCoordinator().getOrCreateTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --

same here


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147962#comment-15147962
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on the pull request:

https://github.com/apache/drill/pull/374#issuecomment-184463952
  
Please let me know of further comments and remember to +1 if you are comfy 
with the patch. If nothing severe is seen, let's take all comments on follow-up 
PRs. This patch is long waiting I'd like to check this in. Thanks for all 
feedback.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147952#comment-15147952
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52958072
  
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -120,7 +120,7 @@ drill.exec: {
 affinity.factor: 1.2
   },
   sys.store.provider: {
-class: "org.apache.drill.exec.store.sys.zk.ZkPStoreProvider",
+class: 
"org.apache.drill.exec.store.sys.store.provider.ZookeeperPersistentStoreProvider",
--- End diff --

All included.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147949#comment-15147949
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52957626
  
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -120,7 +120,7 @@ drill.exec: {
 affinity.factor: 1.2
   },
   sys.store.provider: {
-class: "org.apache.drill.exec.store.sys.zk.ZkPStoreProvider",
+class: 
"org.apache.drill.exec.store.sys.store.provider.ZookeeperPersistentStoreProvider",
--- End diff --

What about other *PStoreProvider? This will still impact those users.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147944#comment-15147944
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52957306
  
--- Diff: 
common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.common.collections;
+
+import java.util.Map;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+
+public class ImmutableEntry implements Map.Entry  {
+  private final K key;
+  private final V value;
+
+  public ImmutableEntry(final K key, final V value) {
+this.key = Preconditions.checkNotNull(key, "key is required");
+this.value = Preconditions.checkNotNull(value, "value is required");
+  }
+
+  @Override
+  public K getKey() {
+return key;
+  }
+
+  @Override
+  public V getValue() {
+return value;
+  }
+
+  @Override
+  public V setValue(final V value) {
+throw new UnsupportedOperationException("entry is immutable");
+  }
+
+  @Override
+  public boolean equals(final Object other) {
+if (other instanceof ImmutableEntry && other.getClass() == getClass()) 
{
+  final ImmutableEntry entry = (ImmutableEntry)other;
+  return Objects.equal(key, entry.key) && Objects.equal(value, 
entry.value);
--- End diff --

Cool. I think we should consider replacing all uses on a separate patch 
once this is deprecated.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147942#comment-15147942
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52957167
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZKClusterCoordinator.java
 ---
@@ -206,6 +209,16 @@ public DistributedSemaphore getSemaphore(String name, 
int maximumLeases) {
 return new ZkDistributedSemaphore(curator, "/semaphore/" + name, 
maximumLeases);
   }
 
+  @Override
+  public  TransientStore getOrCreateTransientStore(final 
TransientStoreConfig config) {
+final ZkEphemeralStore store = 
(ZkEphemeralStore)factory.getOrCreateStore(config);
+try {
+  store.start();
--- End diff --

That logic now is in factory which calls start once.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147920#comment-15147920
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52955476
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/LocalClusterCoordinator.java
 ---
@@ -125,6 +127,11 @@ public DistributedSemaphore getSemaphore(final String 
name, final int maximumLea
 return semaphores.get(name);
   }
 
+  @Override
+  public  TransientStore getOrCreateTransientStore(final 
TransientStoreConfig config) {
+return new MapBackedStore<>(config);
--- End diff --

I think it's quite cheap here to do so here. Added caching too.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147919#comment-15147919
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52955131
  
--- Diff: 
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStoreProvider.java
 ---
@@ -41,10 +39,8 @@
 import org.apache.hadoop.hbase.client.HTableInterface;
 import org.apache.hadoop.hbase.util.Bytes;
 
-import com.google.common.annotations.VisibleForTesting;
-
-public class HBasePStoreProvider implements PStoreProvider {
-  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(HBasePStoreProvider.class);
+public class HBasePersistentStoreProvider extends 
BasePersistentStoreProvider {
+  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(HBasePersistentStoreProvider.class);
--- End diff --

not relevant to my changes but did my best to privatize them all ;)


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147918#comment-15147918
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52955096
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZookeeperClient.java
 ---
@@ -0,0 +1,238 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.zk;
+
+import java.util.Iterator;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
+import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.recipes.cache.ChildData;
+import org.apache.curator.framework.recipes.cache.PathChildrenCache;
+import org.apache.drill.common.collections.ImmutableEntry;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.zookeeper.CreateMode;
+
+/**
+ * A namespace aware Zookeper client.
--- End diff --

seriously :+1: 


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147916#comment-15147916
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52955022
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/LocalClusterCoordinator.java
 ---
@@ -125,6 +127,11 @@ public DistributedSemaphore getSemaphore(final String 
name, final int maximumLea
 return semaphores.get(name);
   }
 
+  @Override
+  public  TransientStore getOrCreateTransientStore(final 
TransientStoreConfig config) {
+return new MapBackedStore<>(config);
--- End diff --

Looks like we are creating a new MapBackedStore for every call to this. 
There is no caching factory in the call path to this method either.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147912#comment-15147912
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52954392
  
--- Diff: 
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStoreProvider.java
 ---
@@ -41,10 +39,8 @@
 import org.apache.hadoop.hbase.client.HTableInterface;
 import org.apache.hadoop.hbase.util.Bytes;
 
-import com.google.common.annotations.VisibleForTesting;
-
-public class HBasePStoreProvider implements PStoreProvider {
-  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(HBasePStoreProvider.class);
+public class HBasePersistentStoreProvider extends 
BasePersistentStoreProvider {
+  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(HBasePersistentStoreProvider.class);
--- End diff --

_private_ for all loggers of *Provider and *Store would be nice :)


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147911#comment-15147911
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52954382
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
 ---
@@ -237,11 +237,17 @@ void unpauseExecutingFragments(final DrillbitContext 
drillbitContext) {
 }
   }
 
+  @Override
+  public void close() throws Exception {
+profileStore.close();
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147908#comment-15147908
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52954150
  
--- Diff: 
common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.common.collections;
+
+import java.util.Map;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+
+public class ImmutableEntry implements Map.Entry  {
+  private final K key;
+  private final V value;
+
+  public ImmutableEntry(final K key, final V value) {
+this.key = Preconditions.checkNotNull(key, "key is required");
+this.value = Preconditions.checkNotNull(value, "value is required");
+  }
+
+  @Override
+  public K getKey() {
+return key;
+  }
+
+  @Override
+  public V getValue() {
+return value;
+  }
+
+  @Override
+  public V setValue(final V value) {
+throw new UnsupportedOperationException("entry is immutable");
+  }
+
+  @Override
+  public boolean equals(final Object other) {
+if (other instanceof ImmutableEntry && other.getClass() == getClass()) 
{
+  final ImmutableEntry entry = (ImmutableEntry)other;
+  return Objects.equal(key, entry.key) && Objects.equal(value, 
entry.value);
--- End diff --

There is no deprecated annotation, but the doc. for the guava 18 that we 
use has this note as well.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147906#comment-15147906
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52954081
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZkEphemeralStore.java
 ---
@@ -0,0 +1,145 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.zk;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Lists;
+import org.apache.curator.framework.CuratorFramework;
+import 
org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
+import org.apache.drill.common.collections.ImmutableEntry;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.exec.coord.store.BaseTransientStore;
+import org.apache.drill.exec.coord.store.TransientStoreConfig;
+import org.apache.drill.exec.coord.store.TransientStoreEvent;
+import org.apache.drill.exec.serialization.InstanceSerializer;
+import org.apache.zookeeper.CreateMode;
+
+public class ZkEphemeralStore extends BaseTransientStore {
+
+  @VisibleForTesting
+  protected final PathChildrenCacheListener dispatcher = new 
EventDispatcher<>(this);
+  private final ZookeeperClient client;
+
+  public ZkEphemeralStore(final TransientStoreConfig config, final 
CuratorFramework curator) {
+super(config);
+this.client = new ZookeeperClient(curator, PathUtils.join("/", 
config.getName()), CreateMode.EPHEMERAL);
+  }
+
+  public void start() throws Exception {
+getClient().getCache().getListenable().addListener(dispatcher);
+getClient().start();
+  }
+
+  protected ZookeeperClient getClient() {
+return client;
+  }
+
+  @Override
+  public V get(final String key) {
+final byte[] bytes = getClient().get(key);
+if (bytes == null) {
+  return null;
+}
+try {
+  return config.getSerializer().deserialize(bytes);
+} catch (final IOException e) {
+  throw new DrillRuntimeException(String.format("unable to deserialize 
value at %s", key), e);
+}
+  }
+
+  @Override
+  public V put(final String key, final V value) {
+final InstanceSerializer serializer = config.getSerializer();
+try {
+  final byte[] old = getClient().get(key);
+  final byte[] bytes = serializer.serialize(value);
+  getClient().put(key, bytes);
+  if (old == null) {
+return null;
+  }
+  return serializer.deserialize(old);
+} catch (final IOException e) {
+  throw new DrillRuntimeException(String.format("unable to 
de/serialize value of type %s", value.getClass()), e);
+}
+  }
+
+  @Override
+  public V putIfAbsent(final String key, final V value) {
+final V old = get(key);
+if (old == null) {
+  try {
+final byte[] bytes = config.getSerializer().serialize(value);
+getClient().put(key, bytes);
+  } catch (final IOException e) {
+throw new DrillRuntimeException(String.format("unable to serialize 
value of type %s", value.getClass()), e);
+  }
+}
+return old;
+  }
+
+  @Override
+  public V remove(final String key) {
+final V existing = get(key);
+if (existing != null) {
+  getClient().delete(key);
+}
+return existing;
+  }
+
+  @Override
+  public Iterator> entries() {
+return 

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147890#comment-15147890
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953099
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/PersistentStore.java
 ---
@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys;
+
+import java.util.Iterator;
+import java.util.Map;
+
+/**
+ * An abstraction used to store and retrieve instances of given value type.
+ *
+ * @param   value type
+ */
+public interface PersistentStore extends AutoCloseable {
--- End diff --

Any reason you chose to remove extending _java.lang.Iterable_? Most usages 
now look like Lists.newArrayList(store.getAll())


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147889#comment-15147889
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953084
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -267,9 +277,8 @@ public String cancelQuery(@PathParam("queryid") String 
queryId) throws IOExcepti
 }
 
 // then check remote running
-try{
-  PStore runningQueries = 
provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-  QueryInfo info = runningQueries.get(queryId);
+try (final TransientStore running = 
getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --

same here


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147886#comment-15147886
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953078
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -146,38 +155,37 @@ public QProfiles(List runningQueries, 
List finishedQue
   @Path("/profiles.json")
   @Produces(MediaType.APPLICATION_JSON)
   public QProfiles getProfilesJSON() {
-PStore completed = null;
-PStore running = null;
-try {
-  completed = provider().getStore(QueryManager.QUERY_PROFILE);
-  running = provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-} catch (IOException e) {
-  logger.debug("Failed to get profiles from persistent or ephemeral 
store.");
-  return new QProfiles(new ArrayList(), new 
ArrayList());
-}
-
-List runningQueries = Lists.newArrayList();
-
-for (Map.Entry entry : running) {
-  QueryInfo profile = entry.getValue();
-  if (principal.canManageProfileOf(profile.getUser())) {
-runningQueries.add(new ProfileInfo(entry.getKey(), 
profile.getStart(), profile.getForeman().getAddress(),
-profile.getQuery(), profile.getState().name(), 
profile.getUser()));
+try (
+final PersistentStore completed = 
getProvider().getStore(QueryManager.QUERY_PROFILE);
+final TransientStore running = 
getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO);
--- End diff --

This isn't creating a new store, right?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147891#comment-15147891
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953108
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/PersistentStoreProvider.java
 ---
@@ -17,17 +17,26 @@
  */
 package org.apache.drill.exec.store.sys;
 
-import java.util.Map;
-
+import org.apache.drill.exec.exception.StoreException;
 
 /**
- * Interface for reading and writing values to a persistent storage 
provider.  Iterators are guaranteed to be returned in key order.
- * @param 
+ * A factory used to create {@link PersistentStore store} instances.
+ *
  */
-public interface PStore extends Iterable> {
-  public V get(String key);
-  public void put(String key, V value);
-  public boolean putIfAbsent(String key, V value);
-  public void delete(String key);
-  public void close();
+public interface PersistentStoreProvider extends AutoCloseable {
+  /**
+   * Gets or creates a {@link PersistentStore persistent store} for the 
given configuration.
+   *
+   * Note that implementors have liberty to cache previous {@link 
PersistentStore store} instances.
+   *
+   * @param config  store configuration
+   * @param   store value type
+   */
+   PersistentStore getStore(PersistentStoreConfig config) throws 
StoreException;
--- End diff --

getOrCreateStore(...)?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147888#comment-15147888
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953080
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -200,9 +208,8 @@ private QueryProfile getQueryProfile(String queryId) 
throws IOException {
 }
 
 // then check remote running
-try{
-  PStore runningQueries = 
provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-  QueryInfo info = runningQueries.get(queryId);
+try (final TransientStore running = 
getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --

same here


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147885#comment-15147885
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953064
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZkEphemeralStore.java
 ---
@@ -0,0 +1,145 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.zk;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Lists;
+import org.apache.curator.framework.CuratorFramework;
+import 
org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
+import org.apache.drill.common.collections.ImmutableEntry;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.exec.coord.store.BaseTransientStore;
+import org.apache.drill.exec.coord.store.TransientStoreConfig;
+import org.apache.drill.exec.coord.store.TransientStoreEvent;
+import org.apache.drill.exec.serialization.InstanceSerializer;
+import org.apache.zookeeper.CreateMode;
+
+public class ZkEphemeralStore extends BaseTransientStore {
+
+  @VisibleForTesting
+  protected final PathChildrenCacheListener dispatcher = new 
EventDispatcher<>(this);
+  private final ZookeeperClient client;
+
+  public ZkEphemeralStore(final TransientStoreConfig config, final 
CuratorFramework curator) {
+super(config);
+this.client = new ZookeeperClient(curator, PathUtils.join("/", 
config.getName()), CreateMode.EPHEMERAL);
+  }
+
+  public void start() throws Exception {
+getClient().getCache().getListenable().addListener(dispatcher);
+getClient().start();
+  }
+
+  protected ZookeeperClient getClient() {
+return client;
+  }
+
+  @Override
+  public V get(final String key) {
+final byte[] bytes = getClient().get(key);
+if (bytes == null) {
+  return null;
+}
+try {
+  return config.getSerializer().deserialize(bytes);
+} catch (final IOException e) {
+  throw new DrillRuntimeException(String.format("unable to deserialize 
value at %s", key), e);
+}
+  }
+
+  @Override
+  public V put(final String key, final V value) {
+final InstanceSerializer serializer = config.getSerializer();
+try {
+  final byte[] old = getClient().get(key);
+  final byte[] bytes = serializer.serialize(value);
+  getClient().put(key, bytes);
+  if (old == null) {
+return null;
+  }
+  return serializer.deserialize(old);
+} catch (final IOException e) {
+  throw new DrillRuntimeException(String.format("unable to 
de/serialize value of type %s", value.getClass()), e);
+}
+  }
+
+  @Override
+  public V putIfAbsent(final String key, final V value) {
+final V old = get(key);
+if (old == null) {
+  try {
+final byte[] bytes = config.getSerializer().serialize(value);
+getClient().put(key, bytes);
+  } catch (final IOException e) {
+throw new DrillRuntimeException(String.format("unable to serialize 
value of type %s", value.getClass()), e);
+  }
+}
+return old;
+  }
+
+  @Override
+  public V remove(final String key) {
+final V existing = get(key);
+if (existing != null) {
+  getClient().delete(key);
+}
+return existing;
+  }
+
+  @Override
+  public Iterator> entries() {
+return 

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147883#comment-15147883
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953044
  
--- Diff: 
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStore.java
 ---
@@ -148,13 +154,13 @@ private void delete(byte[] row) {
 private Result current = null;
 private Result last = null;
 private boolean done = false;
-private int rowsRead = 0;
 
-Iter() {
+Iter(int take) {
   try {
 Scan scan = new Scan(tableNameStartKey, tableNameStopKey);
 scan.addColumn(FAMILY, QUALIFIER);
-scan.setCaching(config.getMaxIteratorSize() > 100 ? 100 : 
config.getMaxIteratorSize());
+scan.setCaching(Math.min(take, 100));
--- End diff --

Is _config.getMaxIteratorSize()_ not required?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147882#comment-15147882
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953040
  
--- Diff: 
common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java ---
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.common.collections;
+
+import java.util.Map;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+
+public class ImmutableEntry implements Map.Entry  {
+  private final K key;
+  private final V value;
+
+  public ImmutableEntry(final K key, final V value) {
+this.key = Preconditions.checkNotNull(key, "key is required");
+this.value = Preconditions.checkNotNull(value, "value is required");
+  }
+
+  @Override
+  public K getKey() {
+return key;
+  }
+
+  @Override
+  public V getValue() {
+return value;
+  }
+
+  @Override
+  public V setValue(final V value) {
+throw new UnsupportedOperationException("entry is immutable");
+  }
+
+  @Override
+  public boolean equals(final Object other) {
+if (other instanceof ImmutableEntry && other.getClass() == getClass()) 
{
+  final ImmutableEntry entry = (ImmutableEntry)other;
+  return Objects.equal(key, entry.key) && Objects.equal(value, 
entry.value);
--- End diff --

google.common.base.Objects#equal and #hashCode [should be treated as 
deprecated](https://github.com/google/guava/blob/master/guava/src/com/google/common/base/Objects.java#L55).


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147884#comment-15147884
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953057
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java
 ---
@@ -0,0 +1,22 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.store;
+
+public interface TransientStoreListener {
--- End diff --

javadoc


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147877#comment-15147877
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on the pull request:

https://github.com/apache/drill/pull/374#issuecomment-184436146
  
I have retracted unrelated mongo changes. Thanks for the feedback.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147866#comment-15147866
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52951541
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java
 ---
@@ -0,0 +1,22 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.store;
+
+public interface TransientStoreListener {
+  void onChange(TransientStoreEvent event);
--- End diff --

I typically prefer this way as it allows extending capabilities(esp event 
types) via polymorphism without requiring to alter the interface. I am fine 
with either way though.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147860#comment-15147860
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52951273
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/PathUtils.java ---
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.zk;
+
+import com.google.common.base.Preconditions;
+import org.apache.parquet.Strings;
+
+public final class PathUtils {
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147857#comment-15147857
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950999
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/ClusterCoordinator.java
 ---
@@ -60,16 +62,23 @@
   public abstract DistributedSemaphore getSemaphore(String name, int 
maximumLeases);
 
   /**
+   * Returns a new {@link TransientStore store} instance with the given 
{@link TransientStoreConfig configuration}.
+   * @param config  store configuration
+   * @param   value type for this store
+   */
+  public abstract  TransientStore 
newTransientStore(TransientStoreConfig config);
+
+  /**
* Actions to take when there are a set of new de-active drillbits.
* @param unregisteredBits
*/
-  public void drillbitUnregistered(Set unregisteredBits) 
{
+  protected void drillbitUnregistered(Set 
unregisteredBits) {
--- End diff --

Wrong use of this method outside of subclasses could easily mess the system 
up. To my understanding, this method should only be visible to implementor who 
is the ultimate authority to fire listeners on topology changes, so protected 
seems to make more sense here.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147855#comment-15147855
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950576
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/MapBackedStore.java
 ---
@@ -0,0 +1,86 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.local;
+
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.coord.store.BaseTransientStore;
+import org.apache.drill.exec.coord.store.TransientStoreConfig;
+import org.apache.drill.exec.coord.store.TransientStoreEvent;
+import org.apache.drill.exec.coord.store.TransientStoreEventType;
+
+public class MapBackedStore extends BaseTransientStore {
+  private final HashMap delegate = Maps.newHashMap();
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147854#comment-15147854
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950554
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/serialization/JacksonSerializer.java
 ---
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.serialization;
+
+import java.io.IOException;
+import java.util.Objects;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.ObjectWriter;
+
+public class JacksonSerializer implements InstanceSerializer {
+  private final ObjectReader reader;
+  private final ObjectWriter writer;
+
+  public JacksonSerializer(final ObjectMapper mapper, final Class 
klazz) {
+this.reader = mapper.reader(klazz);
+this.writer = mapper.writer();
+  }
+
+  @Override
+  public T deserialize(final byte[] raw) throws IOException {
+return reader.readValue(raw);
+  }
+
+  @Override
+  public byte[] serialize(final T instance) throws IOException {
+return writer.writeValueAsBytes(instance);
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+if (obj instanceof JacksonSerializer && 
obj.getClass().equals(getClass())) {
+  final JacksonSerializer other = (JacksonSerializer)obj;
+  return Objects.equals(reader, other.reader) && 
Objects.equals(writer, other.writer);
+}
+return false;
+  }
+
+  @Override
+  public int hashCode() {
+return super.hashCode();
--- End diff --

nice catch. thanks. done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147853#comment-15147853
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950537
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/ZookeeperPersistentStore.java
 ---
@@ -0,0 +1,136 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys.store;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
+import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterators;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.drill.common.collections.ImmutableEntry;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.exec.coord.zk.PathUtils;
+import org.apache.drill.exec.coord.zk.ZookeeperClient;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.serialization.InstanceSerializer;
+import org.apache.drill.exec.store.sys.BasePersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.PersistentStoreMode;
+import org.apache.zookeeper.CreateMode;
+
+/**
+ * This is the abstract class that is shared by ZkPStore (Persistent 
store) and ZkEStore (Ephemeral Store)
--- End diff --

done


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147852#comment-15147852
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950533
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZKClusterCoordinator.java
 ---
@@ -206,6 +205,16 @@ public DistributedSemaphore getSemaphore(String name, 
int maximumLeases) {
 return new ZkDistributedSemaphore(curator, "/semaphore/" + name, 
maximumLeases);
   }
 
+  @Override
+  public  TransientStore newTransientStore(final 
TransientStoreConfig config) {
+final ZkEphemeralStore store = new ZkEphemeralStore<>(config, 
curator);
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15146865#comment-15146865
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52855320
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZKClusterCoordinator.java
 ---
@@ -206,6 +205,16 @@ public DistributedSemaphore getSemaphore(String name, 
int maximumLeases) {
 return new ZkDistributedSemaphore(curator, "/semaphore/" + name, 
maximumLeases);
   }
 
+  @Override
+  public  TransientStore newTransientStore(final 
TransientStoreConfig config) {
+final ZkEphemeralStore store = new ZkEphemeralStore<>(config, 
curator);
--- End diff --

I think you need to cache these since each one will create a listener. This 
used to be done by cachingstoreprovider but I don't think that is in the 
Transient path anymore.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15146861#comment-15146861
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52855239
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/ZookeeperPersistentStore.java
 ---
@@ -0,0 +1,136 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys.store;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
+import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterators;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.drill.common.collections.ImmutableEntry;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.exec.coord.zk.PathUtils;
+import org.apache.drill.exec.coord.zk.ZookeeperClient;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.serialization.InstanceSerializer;
+import org.apache.drill.exec.store.sys.BasePersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.PersistentStoreMode;
+import org.apache.zookeeper.CreateMode;
+
+/**
+ * This is the abstract class that is shared by ZkPStore (Persistent 
store) and ZkEStore (Ephemeral Store)
--- End diff --

Needs update


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15146860#comment-15146860
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52855204
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/serialization/JacksonSerializer.java
 ---
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.serialization;
+
+import java.io.IOException;
+import java.util.Objects;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.ObjectWriter;
+
+public class JacksonSerializer implements InstanceSerializer {
+  private final ObjectReader reader;
+  private final ObjectWriter writer;
+
+  public JacksonSerializer(final ObjectMapper mapper, final Class 
klazz) {
+this.reader = mapper.reader(klazz);
+this.writer = mapper.writer();
+  }
+
+  @Override
+  public T deserialize(final byte[] raw) throws IOException {
+return reader.readValue(raw);
+  }
+
+  @Override
+  public byte[] serialize(final T instance) throws IOException {
+return writer.writeValueAsBytes(instance);
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+if (obj instanceof JacksonSerializer && 
obj.getClass().equals(getClass())) {
+  final JacksonSerializer other = (JacksonSerializer)obj;
+  return Objects.equals(reader, other.reader) && 
Objects.equals(writer, other.writer);
+}
+return false;
+  }
+
+  @Override
+  public int hashCode() {
+return super.hashCode();
--- End diff --

This seems like a bad hashCode since it will have different hashCodes for 
two things that are equal.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15146859#comment-15146859
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52855187
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/serialization/InstanceSerializer.java
 ---
@@ -6,20 +6,20 @@
  * to you under the Apache License, Version 2.0 (the
  * "License"); you may not use this file except in compliance
  * with the License.  You may obtain a copy of the License at
- *
+ * 
  * http://www.apache.org/licenses/LICENSE-2.0
- *
+ * 
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.apache.drill.exec.store.sys.serialize;
+package org.apache.drill.exec.serialization;
 
 import java.io.IOException;
 
-public interface PClassSerializer {
-  public byte[] serialize(X val) throws IOException;
-  public X deserialize(byte[] bytes) throws IOException;
+public interface InstanceSerializer {
+  byte[] serialize(T instance) throws IOException;
--- End diff --

Since you are cleaning this up, I'm thinking that maybe we should 
consolidate this and the CustomSerDe interface I added to CustomTunnel and move 
these some place general (common.serde?)


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15146858#comment-15146858
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52855145
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/PathUtils.java ---
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.zk;
+
+import com.google.common.base.Preconditions;
+import org.apache.parquet.Strings;
+
+public final class PathUtils {
--- End diff --

Would be good to have some javadocs here


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15146857#comment-15146857
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52855121
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java
 ---
@@ -0,0 +1,22 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.store;
+
+public interface TransientStoreListener {
+  void onChange(TransientStoreEvent event);
--- End diff --

What do you think about separate methods for onCreate, onUpdate, onDelete?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15146855#comment-15146855
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52855063
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/MapBackedStore.java
 ---
@@ -0,0 +1,86 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.coord.local;
+
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.coord.store.BaseTransientStore;
+import org.apache.drill.exec.coord.store.TransientStoreConfig;
+import org.apache.drill.exec.coord.store.TransientStoreEvent;
+import org.apache.drill.exec.coord.store.TransientStoreEventType;
+
+public class MapBackedStore extends BaseTransientStore {
+  private final HashMap delegate = Maps.newHashMap();
--- End diff --

I think this needs to be a concurrent map.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15146854#comment-15146854
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52855054
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/ClusterCoordinator.java
 ---
@@ -60,16 +62,23 @@
   public abstract DistributedSemaphore getSemaphore(String name, int 
maximumLeases);
 
   /**
+   * Returns a new {@link TransientStore store} instance with the given 
{@link TransientStoreConfig configuration}.
+   * @param config  store configuration
+   * @param   value type for this store
+   */
+  public abstract  TransientStore 
newTransientStore(TransientStoreConfig config);
+
+  /**
* Actions to take when there are a set of new de-active drillbits.
* @param unregisteredBits
*/
-  public void drillbitUnregistered(Set unregisteredBits) 
{
+  protected void drillbitUnregistered(Set 
unregisteredBits) {
--- End diff --

Why the change to protected?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15145289#comment-15145289
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52797746
  
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -120,7 +120,7 @@ drill.exec: {
 affinity.factor: 1.2
   },
   sys.store.provider: {
-class: "org.apache.drill.exec.store.sys.zk.ZkPStoreProvider",
+class: 
"org.apache.drill.exec.store.sys.store.provider.ZookeeperPersistentStoreProvider",
--- End diff --

Upgrade and downgrade process can be affected by this change (e.g. when 
overridden conf. file has "..._ZkPStoreProvider_"), right?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15145621#comment-15145621
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52817125
  
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -120,7 +120,7 @@ drill.exec: {
 affinity.factor: 1.2
   },
   sys.store.provider: {
-class: "org.apache.drill.exec.store.sys.zk.ZkPStoreProvider",
+class: 
"org.apache.drill.exec.store.sys.store.provider.ZookeeperPersistentStoreProvider",
--- End diff --

Sure. Overriding config with what is already a default 
value(ZkPStoreProvider) seems odd though. Back to your point, this patch 
proposes a major design/API change around old E/PStore so all references to 
ZkPStoreProvider will fail following this patch.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15145639#comment-15145639
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52818257
  
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -120,7 +120,7 @@ drill.exec: {
 affinity.factor: 1.2
   },
   sys.store.provider: {
-class: "org.apache.drill.exec.store.sys.zk.ZkPStoreProvider",
+class: 
"org.apache.drill.exec.store.sys.store.provider.ZookeeperPersistentStoreProvider",
--- End diff --

We do that on the perf. cluster :P

In fact, any other _\*PstoreProvider_ has to change 
_\*PersistentStoreProvider_ after this patch (and reverse if downgrading).


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15142007#comment-15142007
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns closed the pull request at:

https://github.com/apache/drill/pull/325


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15142076#comment-15142076
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

GitHub user hnfgns opened a pull request:

https://github.com/apache/drill/pull/374

DRILL-4275: create TransientStore for short-lived objects; refactor 
PersistentStore to introduce pagination mechanism

ps: removed PR#395 mistakenly so starting over.

collections/
introducing immutable entry

coord/ClusterCoordinator
add a factory method to create transient store

coord/store
introduce transient store and other classes around: factory, config, 
event, event type
introduce base transient store implementation

coord/zk
introducing path utils for zk
introducing general purpose zk client, unit tested
complete rewrite of ZkPersistentStore
complete rewrite of ZkEphemeralStore, unit tested
introducing event dispatcher used by ZkEphemeralStore -- externalized 
for unit testing, unit tested

coord/local/MapBackedStore
introduces a local, map backed transient store

coord/*
updates to adapt new subclasses

serialization/ (both transient & persistent store uses this package)
introducing instance serializer
introducing two concrete implementations: proto and jackson serializers

all of PersistentStore subclasses
implements new pagination logic

java-exec/pom.xml
adds curator-test dependency for unit tests

server/
update so that transient store is acquired, properly closed.

*/
misc renamings to reflect class name changes, to remove unneeded import
misc unit test fixes
misc minor clean-ups

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/hnfgns/incubator-drill DRILL-4275

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/374.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #374


commit e077a2d6ba59a6abfe526bd8f38259d3959be5a7
Author: Hanifi Gunes 
Date:   2016-01-15T01:06:21Z

DRILL-4275: create TransientStore for short-lived objects; refactor 
PersistentStore to introduce pagination mechanism




> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15113507#comment-15113507
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on the pull request:

https://github.com/apache/drill/pull/325#issuecomment-174113221
  
I have created the following 
[doodle](http://doodle.com/poll/zxkxdzfkgfuanw59) to schedule this.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15107553#comment-15107553
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on the pull request:

https://github.com/apache/drill/pull/325#issuecomment-173003718
  
How about anytime tomorrow from 10am till noon?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15105583#comment-15105583
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on the pull request:

https://github.com/apache/drill/pull/325#issuecomment-172604790
  
Cool. Let's sync up on tomorrow's weekly hangout. Let me know if that does 
not work for you.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15106280#comment-15106280
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on the pull request:

https://github.com/apache/drill/pull/325#issuecomment-172747355
  
Let's do a separate hangout. This seems to deep/specific to cover in a
large group.
On Jan 18, 2016 9:51 AM, "Hanifi Gunes"  wrote:

> Cool. Let's sync up on tomorrow's weekly hangout. Let me know if that does
> not work for you.
>
> —
> Reply to this email directly or view it on GitHub
> .
>



> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15103880#comment-15103880
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on the pull request:

https://github.com/apache/drill/pull/325#issuecomment-172369155
  
You want to do a hangout to discuss the concept of EStores and watches and 
the relationship with the ClusterCoordinator interface. We're currently 
actively in the process of implementing a new EStore/PStore pair and I'd like 
to figure out the best way to improve the interface without major disruption to 
the work we're doing.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15102513#comment-15102513
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

GitHub user hnfgns opened a pull request:

https://github.com/apache/drill/pull/325

DRILL-4275: Refactor e/pstore interfaces and their factories to provide a 
unified mechanism to access stores

The main changes are the following:
Remove EStore and EStoreProvider interfaces.
Rename PStore to Store, PStoreProvider to StoreProvider
Extend Store with getMode():StoreMode that returns store persistence level
Ensure StoreProvider.getStore(mode) acts like a factory.
Ensure Store implementations throw StoreException in case of failures and 
handle StoreExceptions

All other files contain very tiny, local, satellite changes like variable 
name updates for name consistency, exception handling et cedera.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/hnfgns/incubator-drill DRILL-4275

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/325.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #325


commit 5b5dcd52242261050cfac2dfd2bbfc64e103cf12
Author: Hanifi Gunes 
Date:   2016-01-15T01:06:21Z

DRILL-4275: Refactor e/pstore interfaces and their factories to provide a 
unified mechanism to access stores




> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15102541#comment-15102541
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on a diff in the pull request:

https://github.com/apache/drill/pull/325#discussion_r49909762
  
--- Diff: 
contrib/storage-mongo/src/main/java/org/apache/drill/exec/store/mongo/MongoGroupScan.java
 ---
@@ -180,39 +179,39 @@ private void init() throws IOException {
 chunksMapping = Maps.newHashMap();
 chunksInverseMapping = Maps.newLinkedHashMap();
 if (isShardedCluster(client)) {
-  MongoDatabase db = client.getDatabase(CONFIG);
-  MongoCollection chunksCollection = 
db.getCollection(CHUNKS);
+  MongoDatabase db = client.getDatabase(DrillMongoConstants.CONFIG);
--- End diff --

Can you use a static import here so we don't have a bunch of extra noise in 
the patch?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15102543#comment-15102543
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user jacques-n commented on the pull request:

https://github.com/apache/drill/pull/325#issuecomment-172106187
  
I don't think we should change the PStore API without some level of 
discussion since it was designed as a public API. Can you share the driving 
requirements that pushed you to propose these changes? I also think renaming 
PStore to PersistentStore could cause user confusion since PersistentStore is 
very generic and could be mistaken for some place where people can store tables.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15102663#comment-15102663
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/325#discussion_r49916936
  
--- Diff: 
contrib/storage-mongo/src/main/java/org/apache/drill/exec/store/mongo/MongoGroupScan.java
 ---
@@ -180,39 +179,39 @@ private void init() throws IOException {
 chunksMapping = Maps.newHashMap();
 chunksInverseMapping = Maps.newLinkedHashMap();
 if (isShardedCluster(client)) {
-  MongoDatabase db = client.getDatabase(CONFIG);
-  MongoCollection chunksCollection = 
db.getCollection(CHUNKS);
+  MongoDatabase db = client.getDatabase(DrillMongoConstants.CONFIG);
--- End diff --

Whole point here was to avoid using an interface for constants and to avoid 
doing a static/wildcard import, following [Google guidelines](

https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports). 
However, if this is taken as a show stopper, I can switch to static imports, 
disliking myself later ;)


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15102696#comment-15102696
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on the pull request:

https://github.com/apache/drill/pull/325#issuecomment-172126518
  
I took the API somewhat Drill internal but you are right about discussing 
this in the public list first. This patch -as the numbers above check out- is 
more of a clean-up than an extension. My plan was to discuss possible changes 
to API going forward once this clean-up patch made its way to master. Here is 
the rationale: I spent quite a while trying to understand the current design, 
event flow in the API in an attempt to find a way for subscribing to store 
events. Especially being notified when an ephemeral node dies just like what 
Zookeeper watches offer. I got confused multiple times with E/PStore and 
E/PStoreProvider distinction at interface level as it seems redundant. That's 
how the idea of this clean-up patch rolled out. 

As for naming, I am open for non-abbreviated alternates. I would invite 
developers relying on this interface to read the documentation first. To this 
end, I will go ahead and extend documentation with possible warning areas. 

Please let me know if you want me to address specific issues with this 
patch.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)