You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by hnfgns <gi...@git.apache.org> on 2016/02/11 01:54:50 UTC
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 <ha...@gmail.com>
Date: 2016-01-15T01:06:21Z
DRILL-4275: create TransientStore for short-lived objects; refactor PersistentStore to introduce pagination mechanism
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
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 <V> TransientStore<V> newTransientStore(final TransientStoreConfig<V> config) {
+ final ZkEphemeralStore<V> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<K, V> implements Map.Entry<K, V> {
+ 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<K, V> entry = (ImmutableEntry<K, V>)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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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<ZKClusterCoordinator> 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 <V> PersistentStore<V> getOrCreateStore(final PersistentStoreConfig<V> config) throws StoreException {
+ switch(config.getMode()){
+ case BLOB_PERSISTENT:
+ return new LocalPersistentStore<>(fs, blobRoot, config);
--- End diff --
LocalPersistentStore in ZookeeperPersistentStoreProvider?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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<ProfileInfo> runningQueries, List<ProfileInfo> finishedQue
@Path("/profiles.json")
@Produces(MediaType.APPLICATION_JSON)
public QProfiles getProfilesJSON() {
- PStore<QueryProfile> completed = null;
- PStore<QueryInfo> 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<ProfileInfo>(), new ArrayList<ProfileInfo>());
- }
-
- List<ProfileInfo> runningQueries = Lists.newArrayList();
-
- for (Map.Entry<String, QueryInfo> 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<QueryProfile> completed = getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE);
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52953622
--- 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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 --
Zookeeper
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52962256
--- 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<ZKClusterCoordinator> 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 <V> PersistentStore<V> getOrCreateStore(final PersistentStoreConfig<V> config) throws StoreException {
+ switch(config.getMode()){
+ case BLOB_PERSISTENT:
+ return new LocalPersistentStore<>(fs, blobRoot, config);
--- End diff --
zk relays blob persistency to fs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
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 <V> value type for this store
+ */
+ public abstract <V> TransientStore<V> newTransientStore(TransientStoreConfig<V> config);
+
+ /**
* Actions to take when there are a set of new de-active drillbits.
* @param unregisteredBits
*/
- public void drillbitUnregistered(Set<DrillbitEndpoint> unregisteredBits) {
+ protected void drillbitUnregistered(Set<DrillbitEndpoint> unregisteredBits) {
--- End diff --
Why the change to protected?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52956166
--- 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 <V> TransientStore<V> getOrCreateTransientStore(final TransientStoreConfig<V> config) {
+ return new MapBackedStore<>(config);
--- End diff --
This would otherwise break embedded mode _running_ query profiles.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52956885
--- 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 --
The new patch includes a deprecated ZkPStoreProvider that is to be removed in 1.7. Now old configuration should work as is. We should also state this out in 1.6 release notes. Let me know if you guys have any other concerns as for compatibility.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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<PersistentStoreConfig<?>, PersistentStore<?>> storeCache = Maps.newConcurrentMap();
+ private final PersistentStoreProvider provider;
+
+ public CachingPersistentStoreProvider(PersistentStoreProvider provider) {
+ this.provider = provider;
+ }
+
+ @SuppressWarnings("unchecked")
+ public <V> PersistentStore<V> getOrCreateStore(final PersistentStoreConfig<V> config) throws StoreException {
--- End diff --
override
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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()?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52953970
--- 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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 --
done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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<PersistentStoreConfig<?>, PersistentStore<?>> storeCache = Maps.newConcurrentMap();
+ private final PersistentStoreProvider provider;
+
+ public CachingPersistentStoreProvider(PersistentStoreProvider provider) {
+ this.provider = provider;
+ }
+
+ @SuppressWarnings("unchecked")
+ public <V> PersistentStore<V> getOrCreateStore(final PersistentStoreConfig<V> 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<V>)newStore;
+ }
+ try {
+ newStore.close();
+ } catch (Exception ex) {
+ throw new StoreException(ex);
+ }
+ }
+
+ return (PersistentStore<V>) 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<K, V> implements Map.Entry<K, V> {
+ 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<K, V> entry = (ImmutableEntry<K, V>)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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 <V> value type
+ */
+public interface PersistentStore<V> extends AutoCloseable {
--- End diff --
Any reason you chose to remove extending _java.lang.Iterable_? Most usages now look like Lists.newArrayList(store.getAll())
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52951701
--- 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
- *
+ * <p/>
* http://www.apache.org/licenses/LICENSE-2.0
- *
+ * <p/>
* 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<X> {
- public byte[] serialize(X val) throws IOException;
- public X deserialize(byte[] bytes) throws IOException;
+public interface InstanceSerializer<T> {
+ byte[] serialize(T instance) throws IOException;
--- End diff --
Sounds good. I can do that on a follow-up issue without messing with RPC on this patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<V> extends BaseTransientStore<V> {
+ private final HashMap<String, V> delegate = Maps.newHashMap();
--- End diff --
I think this needs to be a concurrent map.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52956715
--- 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 <V> TransientStore<V> getOrCreateTransientStore(final TransientStoreConfig<V> config) {
+ final ZkEphemeralStore<V> store = (ZkEphemeralStore<V>)factory.getOrCreateStore(config);
+ try {
+ store.start();
--- End diff --
Accessing profiles will trigger starting the store again, which will cause an "already started" exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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 <V> value type for this store
+ */
+ public abstract <V> TransientStore<V> newTransientStore(TransientStoreConfig<V> config);
+
+ /**
* Actions to take when there are a set of new de-active drillbits.
* @param unregisteredBits
*/
- public void drillbitUnregistered(Set<DrillbitEndpoint> unregisteredBits) {
+ protected void drillbitUnregistered(Set<DrillbitEndpoint> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 <V>
+ * A factory used to create {@link PersistentStore store} instances.
+ *
*/
-public interface PStore<V> extends Iterable<Map.Entry<String, V>> {
- 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 <V> store value type
+ */
+ <V> PersistentStore<V> getStore(PersistentStoreConfig<V> config) throws StoreException;
--- End diff --
getOrCreateStore(...)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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 <V> TransientStore<V> newTransientStore(final TransientStoreConfig<V> config) {
+ final ZkEphemeralStore<V> store = new ZkEphemeralStore<>(config, curator);
--- End diff --
done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/374#issuecomment-184037187
Looks reasonable. I added a few comments.
I think the Mongo interface => class changes should be a separate patch. It adds noise to this patch and is really a completely unrelated issue. I also think it touches a lot of files and wonder whether it should ordered in a special way versus other outstanding mongo patches.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<V> extends BaseTransientStore<V> {
+ private final HashMap<String, V> delegate = Maps.newHashMap();
--- End diff --
done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<T> implements InstanceSerializer<T> {
+ private final ObjectReader reader;
+ private final ObjectWriter writer;
+
+ public JacksonSerializer(final ObjectMapper mapper, final Class<T> 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<T> other = (JacksonSerializer<T>)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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<K, V> implements Map.Entry<K, V> {
+ 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<K, V> entry = (ImmutableEntry<K, V>)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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52960464
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java ---
@@ -267,9 +276,8 @@ public String cancelQuery(@PathParam("queryid") String queryId) throws IOExcepti
}
// then check remote running
- try{
- PStore<QueryInfo> runningQueries = provider().getStore(QueryManager.RUNNING_QUERY_INFO);
- QueryInfo info = runningQueries.get(queryId);
+ try (final TransientStore<QueryInfo> running = getCoordinator().getOrCreateTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --
same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52960439
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java ---
@@ -146,38 +154,37 @@ public QProfiles(List<ProfileInfo> runningQueries, List<ProfileInfo> finishedQue
@Path("/profiles.json")
@Produces(MediaType.APPLICATION_JSON)
public QProfiles getProfilesJSON() {
- PStore<QueryProfile> completed = null;
- PStore<QueryInfo> 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<ProfileInfo>(), new ArrayList<ProfileInfo>());
- }
-
- List<ProfileInfo> runningQueries = Lists.newArrayList();
-
- for (Map.Entry<String, QueryInfo> 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<QueryProfile> completed = getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE);
--- End diff --
This should not use try-with-resources statement, which will close the resources at the end of the block.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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<QueryInfo> runningQueries = provider().getStore(QueryManager.RUNNING_QUERY_INFO);
- QueryInfo info = runningQueries.get(queryId);
+ try (final TransientStore<QueryInfo> running = getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --
same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52953882
--- 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 --
I second on Sudheesh's concern. Unless there is a strong reason that we have to make a breaking change such that "all references to ZkPStoreProvider will fail following this patch", I feel we need to think twice before we make such change. Is this the only way that we can go? Have we assessed the impact to Drill's current users (After all, Drill is post 1.0 release)? Do we have a well-documented plan to help them to migrate, if the backward compatibility is broken in new release?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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()?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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 ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52960717
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/PersistentStoreMode.java ---
@@ -6,22 +6,18 @@
* 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
- *
+ * <p/>
* http://www.apache.org/licenses/LICENSE-2.0
- *
+ * <p/>
* 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;
-
-/**
- * Interface to define the provider which return EStore.
- */
-
-public interface EStoreProvider extends PStoreProvider {
+public enum PersistentStoreMode {
--- End diff --
javadoc
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<V> extends BaseTransientStore<V> {
+
+ @VisibleForTesting
+ protected final PathChildrenCacheListener dispatcher = new EventDispatcher<>(this);
+ private final ZookeeperClient client;
+
+ public ZkEphemeralStore(final TransientStoreConfig<V> 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<V> 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<Map.Entry<String, V>> entries() {
+ return Iterators.transform(getClient().entries(), new Function<Map.Entry<String, byte[]>, Map.Entry<String, V>>() {
+ @Nullable
+ @Override
+ public Map.Entry<String, V> apply(@Nullable Map.Entry<String, byte[]> input) {
+ try {
+ final V value = config.getSerializer().deserialize(input.getValue());
+ return new ImmutableEntry<>(input.getKey(), value);
+ } catch (final IOException e) {
+ throw new DrillRuntimeException(String.format("unable to deserialize value at key %s", input.getKey()), e);
+ }
+ }
+ });
+ }
+
+ @Override
+ public int size() {
+ return Lists.newArrayList(entries()).size();
--- End diff --
yep. made it so that it avoids extraneous deserialization step.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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<QueryProfile> profiles = provider().getStore(QueryManager.QUERY_PROFILE);
- QueryProfile queryProfile = profiles.get(queryId);
- if (queryProfile != null) {
- checkOrThrowProfileViewAuthorization(queryProfile);
- return queryProfile;
+ try (final PersistentStore<QueryProfile> profiles = getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE)) {
--- End diff --
same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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<QueryInfo> runningQueries = provider().getStore(QueryManager.RUNNING_QUERY_INFO);
- QueryInfo info = runningQueries.get(queryId);
+ try (final TransientStore<QueryInfo> running = getCoordinator().getOrCreateTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --
same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52953875
--- 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 --
nope. this patch introduces a way to paginate results via #getRange. this option is no longer needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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<ProfileInfo> runningQueries, List<ProfileInfo> finishedQue
@Path("/profiles.json")
@Produces(MediaType.APPLICATION_JSON)
public QProfiles getProfilesJSON() {
- PStore<QueryProfile> completed = null;
- PStore<QueryInfo> 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<ProfileInfo>(), new ArrayList<ProfileInfo>());
- }
-
- List<ProfileInfo> runningQueries = Lists.newArrayList();
-
- for (Map.Entry<String, QueryInfo> 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<QueryProfile> completed = getProvider().getStore(QueryManager.QUERY_PROFILE);
+ final TransientStore<QueryInfo> running = getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO);
--- End diff --
This isn't creating a new store, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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 <V> TransientStore<V> getOrCreateTransientStore(final TransientStoreConfig<V> config) {
+ final ZkEphemeralStore<V> store = (ZkEphemeralStore<V>)factory.getOrCreateStore(config);
+ try {
+ store.start();
--- End diff --
That logic now is in factory which calls start once.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52954225
--- 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 <V> value type
+ */
+public interface PersistentStore<V> extends AutoCloseable {
--- End diff --
Mainly because of pagination and being more explicit both at interface and pagination control level.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52953137
--- 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 --
AutoCloseables.close(...)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<T> implements InstanceSerializer<T> {
+ private final ObjectReader reader;
+ private final ObjectWriter writer;
+
+ public JacksonSerializer(final ObjectMapper mapper, final Class<T> 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<T> other = (JacksonSerializer<T>)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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/374
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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<QueryInfo> runningQueries = provider().getStore(QueryManager.RUNNING_QUERY_INFO);
- QueryInfo info = runningQueries.get(queryId);
+ try (final TransientStore<QueryInfo> running = getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --
same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52954122
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java ---
@@ -146,38 +155,37 @@ public QProfiles(List<ProfileInfo> runningQueries, List<ProfileInfo> finishedQue
@Path("/profiles.json")
@Produces(MediaType.APPLICATION_JSON)
public QProfiles getProfilesJSON() {
- PStore<QueryProfile> completed = null;
- PStore<QueryInfo> 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<ProfileInfo>(), new ArrayList<ProfileInfo>());
- }
-
- List<ProfileInfo> runningQueries = Lists.newArrayList();
-
- for (Map.Entry<String, QueryInfo> 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<QueryProfile> completed = getProvider().getStore(QueryManager.QUERY_PROFILE);
+ final TransientStore<QueryInfo> running = getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO);
--- End diff --
nope instances are now cached. I will rename the method to reflect that change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 <V> TransientStore<V> getOrCreateTransientStore(final TransientStoreConfig<V> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by jacques-n <gi...@git.apache.org>.
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
- *
+ * <p/>
* http://www.apache.org/licenses/LICENSE-2.0
- *
+ * <p/>
* 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<X> {
- public byte[] serialize(X val) throws IOException;
- public X deserialize(byte[] bytes) throws IOException;
+public interface InstanceSerializer<T> {
+ 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?)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<V> extends BaseTransientStore<V> {
+
+ @VisibleForTesting
+ protected final PathChildrenCacheListener dispatcher = new EventDispatcher<>(this);
+ private final ZookeeperClient client;
+
+ public ZkEphemeralStore(final TransientStoreConfig<V> 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<V> 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<Map.Entry<String, V>> entries() {
+ return Iterators.transform(getClient().entries(), new Function<Map.Entry<String, byte[]>, Map.Entry<String, V>>() {
+ @Nullable
+ @Override
+ public Map.Entry<String, V> apply(@Nullable Map.Entry<String, byte[]> input) {
+ try {
+ final V value = config.getSerializer().deserialize(input.getValue());
+ return new ImmutableEntry<>(input.getKey(), value);
+ } catch (final IOException e) {
+ throw new DrillRuntimeException(String.format("unable to deserialize value at key %s", input.getKey()), e);
+ }
+ }
+ });
+ }
+
+ @Override
+ public int size() {
+ return Lists.newArrayList(entries()).size();
--- End diff --
Isn't this expensive?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
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 <V> TransientStore<V> getOrCreateTransientStore(final TransientStoreConfig<V> config) {
+ return new MapBackedStore<>(config);
--- End diff --
I think it's quite cheap here to do so here. Added caching too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52954240
--- 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 <V>
+ * A factory used to create {@link PersistentStore store} instances.
+ *
*/
-public interface PStore<V> extends Iterable<Map.Entry<String, V>> {
- 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 <V> store value type
+ */
+ <V> PersistentStore<V> getStore(PersistentStoreConfig<V> config) throws StoreException;
--- End diff --
done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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<PersistentStoreConfig<?>, PersistentStore<?>> storeCache = Maps.newConcurrentMap();
+ private final PersistentStoreProvider provider;
+
+ public CachingPersistentStoreProvider(PersistentStoreProvider provider) {
+ this.provider = provider;
+ }
+
+ @SuppressWarnings("unchecked")
+ public <V> PersistentStore<V> getOrCreateStore(final PersistentStoreConfig<V> 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<V>)newStore;
+ }
+ try {
+ newStore.close();
+ } catch (Exception ex) {
+ throw new StoreException(ex);
+ }
+ }
+
+ return (PersistentStore<V>) 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());
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/374#discussion_r52953797
--- 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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<K, V> implements Map.Entry<K, V> {
+ 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<K, V> entry = (ImmutableEntry<K, V>)other;
+ return Objects.equal(key, entry.key) && Objects.equal(value, entry.value);
--- End diff --
That sure makes sense. You are probably looking at the cutting edge. We are on guava-18 that does not deprecate these. Thanks for the reminder. Going forward we should keep that in mind.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---