[GitHub] drill pull request: DRILL-4287: During initial DrillTable creation...
Github user jinfengni commented on a diff in the pull request: https://github.com/apache/drill/pull/376#discussion_r52973451 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java --- @@ -529,6 +549,36 @@ public long getRowCount() { } + + // Create and return a new file selection based on reading the metadata cache file. + // This function also initializes a few of ParquetGroupScan's fields as appropriate. + private FileSelection + initFromMetadataCache(DrillFileSystem fs, FileSelection selection) throws IOException { +FileStatus metaRootDir = selection.getFirstPath(fs); +Path metaFilePath = new Path(metaRootDir.getPath(), Metadata.METADATA_FILENAME); + +// get (and set internal field) the metadata for the directory by reading the metadata file +this.parquetTableMetadata = Metadata.readBlockMeta(fs, metaFilePath.toString()); +List fileNames = Lists.newArrayList(); +for (Metadata.ParquetFileMetadata file : parquetTableMetadata.getFiles()) { + fileNames.add(file.getPath()); +} +// when creating the file selection, set the selection root in the form /a/b instead of +// file:/a/b. The reason is that the file names above have been created in the form +// /a/b/c.parquet and the format of the selection root must match that of the file names +// otherwise downstream operations such as partition pruning can break. +final Path metaRootPath = Path.getPathWithoutSchemeAndAuthority(metaRootDir.getPath()); +this.selectionRoot = metaRootPath.toString(); + +// Use the FileSelection constructor directly here instead of the FileSelection.create() method +// because create() changes the root to include the scheme and authority; In future, if create() +// is the preferred way to instantiate a file selection, we may need to do something different... +FileSelection newSelection = new FileSelection(selection.getStatuses(fs), fileNames, metaRootPath.toString()); --- End diff -- Seems the first argument : selection.getStatuses(fs) is the file status before being expanded from cache, while the second argument fileNames represents the expanded file names from cache. Will such difference cause any problem? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4287: During initial DrillTable creation...
Github user jinfengni commented on a diff in the pull request: https://github.com/apache/drill/pull/376#discussion_r52973315 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java --- @@ -68,6 +79,7 @@ protected FileSelection(final FileSelection selection) { this.statuses = selection.statuses; this.files = selection.files; this.selectionRoot = selection.selectionRoot; +this.dirStatus = new BitSet(StatusType.values().length); --- End diff -- Should this constructor copy the dirStatus from the input "selection" 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Question about generating the markdown pages for the Drill site
Hello Devs, I am currently trying to publish a blog post for the 1.5 release before I send out the announcement to the mailing list, and I am having a little trouble with the Jekyll site generation. I pulled down the latest gh-pages branch and wrote a new blog post based on a draft from Bridget: https://github.com/jaltekruse/incubator-drill/commit/0e88b88a6acd4dcfe61ef849c4ed13aa878485d7 When I went to go generate the site using the commands from the readme on the gh-pages branch I am getting errors for old doc pages that are missing dates. I can fix each of the pages by editing it and committing to cause the date auto-generation, but it appears that there are quite a few pages missing the dates. Are these the up-to-date commands that have been used recently? Is there something I should be doing to make it skip the missing dates? (taken from the readme page) jekyll build --config _config.yml,_config-prod.yml _tools/createdatadocs.py jekyll build --config _config.yml,_config-prod.yml Thanks in advance for any help! Jason
[GitHub] drill pull request: DRILL-4287: During initial DrillTable creation...
Github user amansinha100 commented on the pull request: https://github.com/apache/drill/pull/376#issuecomment-184481180 I have incorporated the review comments and updated the PR. Please take a look when you get a chance. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 registry) throws StoreException { +this(registry.getConfig(), registry.getCoordinator().getCurator()); + } + + @VisibleForTesting + public ZookeeperPersistentStoreProvider(final DrillConfig config, final CuratorFramework curator) throws StoreException { +this.curator = curator; + +if (config.hasPath(DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT)) { + blobRoot = new Path(config.getString(DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT)); +}else{ + blobRoot = LocalPersistentStore.getLogDir(); +} + +try { + this.fs = LocalPersistentStore.getFileSystem(config, blobRoot); +} catch (IOException ex) { + throw new StoreException("unable to get filesystem", ex); +} + } + + @Override + public PersistentStore getOrCreateStore(final PersistentStoreConfig config) throws StoreException { +switch(config.getMode()){ +case BLOB_PERSISTENT: + return new LocalPersistentStore<>(fs, blobRoot, config); --- End diff -- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 ConcurrentMapstoreCache = Maps.newConcurrentMap(); + private final PersistentStoreProvider provider; + + public CachingPersistentStoreProvider(PersistentStoreProvider provider) { +this.provider = provider; + } + + @SuppressWarnings("unchecked") + public PersistentStore getOrCreateStore(final PersistentStoreConfig config) throws StoreException { +final PersistentStore store = storeCache.get(config); +if (store == null) { + final PersistentStore newStore = provider.getOrCreateStore(config); + final PersistentStore finalStore = storeCache.putIfAbsent(config, newStore); + if (finalStore == null) { +return (PersistentStore)newStore; + } + try { +newStore.close(); + } catch (Exception ex) { +throw new StoreException(ex); + } +} + +return (PersistentStore) store; + + } + + @Override + public void start() throws Exception { +provider.start(); + } + + @Override + public void close() throws Exception { +for(final PersistentStore store : storeCache.values()){ + store.close(); --- End diff -- AutoCloseables.close(storecache.values() + provider). Done. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52962000 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java --- @@ -146,38 +154,37 @@ public QProfiles(List runningQueries, List finishedQue @Path("/profiles.json") @Produces(MediaType.APPLICATION_JSON) public QProfiles getProfilesJSON() { -PStore completed = null; -PStore running = null; -try { - completed = provider().getStore(QueryManager.QUERY_PROFILE); - running = provider().getStore(QueryManager.RUNNING_QUERY_INFO); -} catch (IOException e) { - logger.debug("Failed to get profiles from persistent or ephemeral store."); - return new QProfiles(new ArrayList(), new ArrayList()); -} - -List runningQueries = Lists.newArrayList(); - -for (Map.Entryentry : running) { - QueryInfo profile = entry.getValue(); - if (principal.canManageProfileOf(profile.getUser())) { -runningQueries.add(new ProfileInfo(entry.getKey(), profile.getStart(), profile.getForeman().getAddress(), -profile.getQuery(), profile.getState().name(), profile.getUser())); +try ( +final PersistentStore completed = getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE); --- End diff -- Fixed. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 ConcurrentMapstoreCache = Maps.newConcurrentMap(); + private final PersistentStoreProvider provider; + + public CachingPersistentStoreProvider(PersistentStoreProvider provider) { +this.provider = provider; + } + + @SuppressWarnings("unchecked") + public PersistentStore getOrCreateStore(final PersistentStoreConfig config) throws StoreException { +final PersistentStore store = storeCache.get(config); +if (store == null) { + final PersistentStore newStore = provider.getOrCreateStore(config); + final PersistentStore finalStore = storeCache.putIfAbsent(config, newStore); + if (finalStore == null) { +return (PersistentStore)newStore; + } + try { +newStore.close(); + } catch (Exception ex) { +throw new StoreException(ex); + } +} + +return (PersistentStore) store; + + } + + @Override + public void start() throws Exception { +provider.start(); + } + + @Override + public void close() throws Exception { +for(final PersistentStore store : storeCache.values()){ + store.close(); --- End diff -- AutoCloseables.close(storeCache.values()); --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 - * + * * 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; - -/** - * 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52960723 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/ZookeeperPersistentStoreProvider.java --- @@ -0,0 +1,85 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.store.sys.store.provider; + +import java.io.IOException; + +import com.google.common.annotations.VisibleForTesting; +import org.apache.curator.framework.CuratorFramework; +import org.apache.drill.common.config.DrillConfig; +import org.apache.drill.exec.coord.zk.ZKClusterCoordinator; +import org.apache.drill.exec.exception.StoreException; +import org.apache.drill.exec.store.dfs.DrillFileSystem; +import org.apache.drill.exec.store.sys.PersistentStore; +import org.apache.drill.exec.store.sys.PersistentStoreRegistry; +import org.apache.drill.exec.store.sys.PersistentStoreConfig; +import org.apache.drill.exec.store.sys.store.LocalPersistentStore; +import org.apache.drill.exec.store.sys.store.ZookeeperPersistentStore; +import org.apache.hadoop.fs.Path; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class ZookeeperPersistentStoreProvider extends BasePersistentStoreProvider { + private static final Logger logger = LoggerFactory.getLogger(ZookeeperPersistentStoreProvider.class); + + private static final String DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT = "drill.exec.sys.store.provider.zk.blobroot"; + + private final CuratorFramework curator; + private final DrillFileSystem fs; + private final Path blobRoot; + + public ZookeeperPersistentStoreProvider(final PersistentStoreRegistry registry) throws StoreException { +this(registry.getConfig(), registry.getCoordinator().getCurator()); + } + + @VisibleForTesting + public ZookeeperPersistentStoreProvider(final DrillConfig config, final CuratorFramework curator) throws StoreException { +this.curator = curator; + +if (config.hasPath(DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT)) { + blobRoot = new Path(config.getString(DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT)); +}else{ + blobRoot = LocalPersistentStore.getLogDir(); +} + +try { + this.fs = LocalPersistentStore.getFileSystem(config, blobRoot); +} catch (IOException ex) { + throw new StoreException("unable to get filesystem", ex); +} + } + + @Override + public PersistentStore getOrCreateStore(final PersistentStoreConfig config) throws StoreException { +switch(config.getMode()){ +case BLOB_PERSISTENT: + return new LocalPersistentStore<>(fs, blobRoot, config); --- End diff -- LocalPersistentStore in ZookeeperPersistentStoreProvider? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52960456 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java --- @@ -217,11 +223,14 @@ private QueryProfile getQueryProfile(String queryId) throws IOException { } // then check blob store -PStore profiles = provider().getStore(QueryManager.QUERY_PROFILE); -QueryProfile queryProfile = profiles.get(queryId); -if (queryProfile != null) { - checkOrThrowProfileViewAuthorization(queryProfile); - return queryProfile; +try (final PersistentStore profiles = getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE)) { --- End diff -- same here --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 runningQueries = provider().getStore(QueryManager.RUNNING_QUERY_INFO); - QueryInfo info = runningQueries.get(queryId); +try (final TransientStore running = getCoordinator().getOrCreateTransientStore(QueryManager.RUNNING_QUERY_INFO)) { --- End diff -- same here --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52960453 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java --- @@ -200,9 +207,8 @@ private QueryProfile getQueryProfile(String queryId) throws IOException { } // then check remote running -try{ - PStore runningQueries = provider().getStore(QueryManager.RUNNING_QUERY_INFO); - QueryInfo info = runningQueries.get(queryId); +try (final TransientStore running = getCoordinator().getOrCreateTransientStore(QueryManager.RUNNING_QUERY_INFO)) { --- End diff -- same here --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 runningQueries, List finishedQue @Path("/profiles.json") @Produces(MediaType.APPLICATION_JSON) public QProfiles getProfilesJSON() { -PStore completed = null; -PStore running = null; -try { - completed = provider().getStore(QueryManager.QUERY_PROFILE); - running = provider().getStore(QueryManager.RUNNING_QUERY_INFO); -} catch (IOException e) { - logger.debug("Failed to get profiles from persistent or ephemeral store."); - return new QProfiles(new ArrayList(), new ArrayList()); -} - -List runningQueries = Lists.newArrayList(); - -for (Map.Entryentry : running) { - QueryInfo profile = entry.getValue(); - if (principal.canManageProfileOf(profile.getUser())) { -runningQueries.add(new ProfileInfo(entry.getKey(), profile.getStart(), profile.getForeman().getAddress(), -profile.getQuery(), profile.getState().name(), profile.getUser())); +try ( +final PersistentStore completed = getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE); --- End diff -- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52957306 --- Diff: common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java --- @@ -0,0 +1,62 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.common.collections; + +import java.util.Map; + +import com.google.common.base.Objects; +import com.google.common.base.Preconditions; + +public class ImmutableEntryimplements Map.Entry { + private final K key; + private final V value; + + public ImmutableEntry(final K key, final V value) { +this.key = Preconditions.checkNotNull(key, "key is required"); +this.value = Preconditions.checkNotNull(value, "value is required"); + } + + @Override + public K getKey() { +return key; + } + + @Override + public V getValue() { +return value; + } + + @Override + public V setValue(final V value) { +throw new UnsupportedOperationException("entry is immutable"); + } + + @Override + public boolean equals(final Object other) { +if (other instanceof ImmutableEntry && other.getClass() == getClass()) { + final ImmutableEntry entry = (ImmutableEntry )other; + return Objects.equal(key, entry.key) && Objects.equal(value, entry.value); --- End diff -- Cool. I think we should consider replacing all uses on a separate patch once this is deprecated. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52957167 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZKClusterCoordinator.java --- @@ -206,6 +209,16 @@ public DistributedSemaphore getSemaphore(String name, int maximumLeases) { return new ZkDistributedSemaphore(curator, "/semaphore/" + name, maximumLeases); } + @Override + public TransientStore getOrCreateTransientStore(final TransientStoreConfig config) { +final ZkEphemeralStore store = (ZkEphemeralStore)factory.getOrCreateStore(config); +try { + store.start(); --- End diff -- That logic now is in factory which calls start once. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 TransientStore getOrCreateTransientStore(final TransientStoreConfig config) { +final ZkEphemeralStore store = (ZkEphemeralStore)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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 TransientStore getOrCreateTransientStore(final TransientStoreConfig 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52955476 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/LocalClusterCoordinator.java --- @@ -125,6 +127,11 @@ public DistributedSemaphore getSemaphore(final String name, final int maximumLea return semaphores.get(name); } + @Override + public TransientStore getOrCreateTransientStore(final TransientStoreConfig config) { +return new MapBackedStore<>(config); --- End diff -- I think it's quite cheap here to do so here. Added caching too. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52955096 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZookeeperClient.java --- @@ -0,0 +1,238 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.coord.zk; + +import java.util.Iterator; +import java.util.Map; + +import javax.annotation.Nullable; + +import com.google.common.base.Function; +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import com.google.common.collect.Iterables; +import org.apache.curator.framework.CuratorFramework; +import org.apache.curator.framework.recipes.cache.ChildData; +import org.apache.curator.framework.recipes.cache.PathChildrenCache; +import org.apache.drill.common.collections.ImmutableEntry; +import org.apache.drill.common.exceptions.DrillRuntimeException; +import org.apache.zookeeper.CreateMode; + +/** + * A namespace aware Zookeper client. --- End diff -- seriously :+1: --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 value type + */ +public interface PersistentStore 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 + * A factory used to create {@link PersistentStore store} instances. + * */ -public interface PStore extends Iterable> { - public V get(String key); - public void put(String key, V value); - public boolean putIfAbsent(String key, V value); - public void delete(String key); - public void close(); +public interface PersistentStoreProvider extends AutoCloseable { + /** + * Gets or creates a {@link PersistentStore persistent store} for the given configuration. + * + * Note that implementors have liberty to cache previous {@link PersistentStore store} instances. + * + * @param config store configuration + * @param store value type + */ + PersistentStore getStore(PersistentStoreConfig config) throws StoreException; --- End diff -- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52954150 --- Diff: common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java --- @@ -0,0 +1,62 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.common.collections; + +import java.util.Map; + +import com.google.common.base.Objects; +import com.google.common.base.Preconditions; + +public class ImmutableEntryimplements Map.Entry { + private final K key; + private final V value; + + public ImmutableEntry(final K key, final V value) { +this.key = Preconditions.checkNotNull(key, "key is required"); +this.value = Preconditions.checkNotNull(value, "value is required"); + } + + @Override + public K getKey() { +return key; + } + + @Override + public V getValue() { +return value; + } + + @Override + public V setValue(final V value) { +throw new UnsupportedOperationException("entry is immutable"); + } + + @Override + public boolean equals(final Object other) { +if (other instanceof ImmutableEntry && other.getClass() == getClass()) { + final ImmutableEntry entry = (ImmutableEntry )other; + return Objects.equal(key, entry.key) && Objects.equal(value, entry.value); --- End diff -- There is no deprecated annotation, but the doc. for the guava 18 that we use has this note as well. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 runningQueries, List finishedQue @Path("/profiles.json") @Produces(MediaType.APPLICATION_JSON) public QProfiles getProfilesJSON() { -PStore completed = null; -PStore running = null; -try { - completed = provider().getStore(QueryManager.QUERY_PROFILE); - running = provider().getStore(QueryManager.RUNNING_QUERY_INFO); -} catch (IOException e) { - logger.debug("Failed to get profiles from persistent or ephemeral store."); - return new QProfiles(new ArrayList(), new ArrayList()); -} - -List runningQueries = Lists.newArrayList(); - -for (Map.Entryentry : running) { - QueryInfo profile = entry.getValue(); - if (principal.canManageProfileOf(profile.getUser())) { -runningQueries.add(new ProfileInfo(entry.getKey(), profile.getStart(), profile.getForeman().getAddress(), -profile.getQuery(), profile.getState().name(), profile.getUser())); +try ( +final PersistentStore completed = getProvider().getStore(QueryManager.QUERY_PROFILE); +final TransientStore running = getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO); --- End diff -- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.coord.store; + +public interface TransientStoreListener { --- End diff -- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.common.collections; + +import java.util.Map; + +import com.google.common.base.Objects; +import com.google.common.base.Preconditions; + +public class ImmutableEntryimplements Map.Entry { + private final K key; + private final V value; + + public ImmutableEntry(final K key, final V value) { +this.key = Preconditions.checkNotNull(key, "key is required"); +this.value = Preconditions.checkNotNull(value, "value is required"); + } + + @Override + public K getKey() { +return key; + } + + @Override + public V getValue() { +return value; + } + + @Override + public V setValue(final V value) { +throw new UnsupportedOperationException("entry is immutable"); + } + + @Override + public boolean equals(final Object other) { +if (other instanceof ImmutableEntry && other.getClass() == getClass()) { + final ImmutableEntry entry = (ImmutableEntry )other; + return Objects.equal(key, entry.key) && Objects.equal(value, entry.value); --- End diff -- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.coord.zk; + +import java.util.Iterator; +import java.util.Map; + +import javax.annotation.Nullable; + +import com.google.common.base.Function; +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import com.google.common.collect.Iterables; +import org.apache.curator.framework.CuratorFramework; +import org.apache.curator.framework.recipes.cache.ChildData; +import org.apache.curator.framework.recipes.cache.PathChildrenCache; +import org.apache.drill.common.collections.ImmutableEntry; +import org.apache.drill.common.exceptions.DrillRuntimeException; +import org.apache.zookeeper.CreateMode; + +/** + * A namespace aware Zookeper client. --- End diff -- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52953108 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/PersistentStoreProvider.java --- @@ -17,17 +17,26 @@ */ package org.apache.drill.exec.store.sys; -import java.util.Map; - +import org.apache.drill.exec.exception.StoreException; /** - * Interface for reading and writing values to a persistent storage provider. Iterators are guaranteed to be returned in key order. - * @param + * A factory used to create {@link PersistentStore store} instances. + * */ -public interface PStore extends Iterable> { - public V get(String key); - public void put(String key, V value); - public boolean putIfAbsent(String key, V value); - public void delete(String key); - public void close(); +public interface PersistentStoreProvider extends AutoCloseable { + /** + * Gets or creates a {@link PersistentStore persistent store} for the given configuration. + * + * Note that implementors have liberty to cache previous {@link PersistentStore store} instances. + * + * @param config store configuration + * @param store value type + */ + PersistentStore getStore(PersistentStoreConfig config) throws StoreException; --- End diff -- getOrCreateStore(...)? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52953084 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java --- @@ -267,9 +277,8 @@ public String cancelQuery(@PathParam("queryid") String queryId) throws IOExcepti } // then check remote running -try{ - PStore runningQueries = provider().getStore(QueryManager.RUNNING_QUERY_INFO); - QueryInfo info = runningQueries.get(queryId); +try (final TransientStore running = getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO)) { --- End diff -- same here --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52953078 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java --- @@ -146,38 +155,37 @@ public QProfiles(List runningQueries, List finishedQue @Path("/profiles.json") @Produces(MediaType.APPLICATION_JSON) public QProfiles getProfilesJSON() { -PStore completed = null; -PStore running = null; -try { - completed = provider().getStore(QueryManager.QUERY_PROFILE); - running = provider().getStore(QueryManager.RUNNING_QUERY_INFO); -} catch (IOException e) { - logger.debug("Failed to get profiles from persistent or ephemeral store."); - return new QProfiles(new ArrayList(), new ArrayList()); -} - -List runningQueries = Lists.newArrayList(); - -for (Map.Entryentry : running) { - QueryInfo profile = entry.getValue(); - if (principal.canManageProfileOf(profile.getUser())) { -runningQueries.add(new ProfileInfo(entry.getKey(), profile.getStart(), profile.getForeman().getAddress(), -profile.getQuery(), profile.getState().name(), profile.getUser())); +try ( +final PersistentStore completed = getProvider().getStore(QueryManager.QUERY_PROFILE); +final TransientStore running = getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO); --- End diff -- This isn't creating a new store, right? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52953099 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/PersistentStore.java --- @@ -0,0 +1,77 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.store.sys; + +import java.util.Iterator; +import java.util.Map; + +/** + * An abstraction used to store and retrieve instances of given value type. + * + * @param value type + */ +public interface PersistentStore extends AutoCloseable { --- End diff -- Any reason you chose to remove extending _java.lang.Iterable_? Most usages now look like Lists.newArrayList(store.getAll()) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52953080 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java --- @@ -200,9 +208,8 @@ private QueryProfile getQueryProfile(String queryId) throws IOException { } // then check remote running -try{ - PStore runningQueries = provider().getStore(QueryManager.RUNNING_QUERY_INFO); - QueryInfo info = runningQueries.get(queryId); +try (final TransientStore running = getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO)) { --- End diff -- same here --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52953064 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZkEphemeralStore.java --- @@ -0,0 +1,145 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.coord.zk; + +import java.io.IOException; +import java.util.Iterator; +import java.util.Map; + +import javax.annotation.Nullable; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Function; +import com.google.common.collect.Iterators; +import com.google.common.collect.Lists; +import org.apache.curator.framework.CuratorFramework; +import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener; +import org.apache.drill.common.collections.ImmutableEntry; +import org.apache.drill.common.exceptions.DrillRuntimeException; +import org.apache.drill.exec.coord.store.BaseTransientStore; +import org.apache.drill.exec.coord.store.TransientStoreConfig; +import org.apache.drill.exec.coord.store.TransientStoreEvent; +import org.apache.drill.exec.serialization.InstanceSerializer; +import org.apache.zookeeper.CreateMode; + +public class ZkEphemeralStore extends BaseTransientStore { + + @VisibleForTesting + protected final PathChildrenCacheListener dispatcher = new EventDispatcher<>(this); + private final ZookeeperClient client; + + public ZkEphemeralStore(final TransientStoreConfig config, final CuratorFramework curator) { +super(config); +this.client = new ZookeeperClient(curator, PathUtils.join("/", config.getName()), CreateMode.EPHEMERAL); + } + + public void start() throws Exception { +getClient().getCache().getListenable().addListener(dispatcher); +getClient().start(); + } + + protected ZookeeperClient getClient() { +return client; + } + + @Override + public V get(final String key) { +final byte[] bytes = getClient().get(key); +if (bytes == null) { + return null; +} +try { + return config.getSerializer().deserialize(bytes); +} catch (final IOException e) { + throw new DrillRuntimeException(String.format("unable to deserialize value at %s", key), e); +} + } + + @Override + public V put(final String key, final V value) { +final InstanceSerializer serializer = config.getSerializer(); +try { + final byte[] old = getClient().get(key); + final byte[] bytes = serializer.serialize(value); + getClient().put(key, bytes); + if (old == null) { +return null; + } + return serializer.deserialize(old); +} catch (final IOException e) { + throw new DrillRuntimeException(String.format("unable to de/serialize value of type %s", value.getClass()), e); +} + } + + @Override + public V putIfAbsent(final String key, final V value) { +final V old = get(key); +if (old == null) { + try { +final byte[] bytes = config.getSerializer().serialize(value); +getClient().put(key, bytes); + } catch (final IOException e) { +throw new DrillRuntimeException(String.format("unable to serialize value of type %s", value.getClass()), e); + } +} +return old; + } + + @Override + public V remove(final String key) { +final V existing = get(key); +if (existing != null) { + getClient().delete(key); +} +return existing; + } + + @Override + public Iterator> entries() { +return Iterators.transform(getClient().entries(), new Function , Map.Entry >() { + @Nullable + @Override + public Map.Entry apply(@Nullable Map.Entry input) { +try {
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52953057 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java --- @@ -0,0 +1,22 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.coord.store; + +public interface TransientStoreListener { --- End diff -- javadoc --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user sudheeshkatkam commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52953040 --- Diff: common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java --- @@ -0,0 +1,62 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.common.collections; + +import java.util.Map; + +import com.google.common.base.Objects; +import com.google.common.base.Preconditions; + +public class ImmutableEntryimplements Map.Entry { + private final K key; + private final V value; + + public ImmutableEntry(final K key, final V value) { +this.key = Preconditions.checkNotNull(key, "key is required"); +this.value = Preconditions.checkNotNull(value, "value is required"); + } + + @Override + public K getKey() { +return key; + } + + @Override + public V getValue() { +return value; + } + + @Override + public V setValue(final V value) { +throw new UnsupportedOperationException("entry is immutable"); + } + + @Override + public boolean equals(final Object other) { +if (other instanceof ImmutableEntry && other.getClass() == getClass()) { + final ImmutableEntry entry = (ImmutableEntry )other; + return Objects.equal(key, entry.key) && Objects.equal(value, entry.value); --- End diff -- google.common.base.Objects#equal and #hashCode [should be treated as deprecated](https://github.com/google/guava/blob/master/guava/src/com/google/common/base/Objects.java#L55). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Created] (DRILL-4386) Unify and move serialization logic to common module or package
Hanifi Gunes created DRILL-4386: --- Summary: Unify and move serialization logic to common module or package Key: DRILL-4386 URL: https://issues.apache.org/jira/browse/DRILL-4386 Project: Apache Drill Issue Type: Improvement Components: Execution - Codegen Affects Versions: 1.5.0 Reporter: Hanifi Gunes Priority: Minor In many places around Drill we rely on custom SerDes. However, there seems some redundancy. For instance, Transient/PersistentStore(introduced by DRILL-4275) relies on InstanceSerializer whereas Controller uses CustomSerDe interface. Effectively these two are the same. We need to unify use cases, possibly moving SerDe logic to common module or some level up in the exec module. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.drill.exec.store.sys.serialize; +package org.apache.drill.exec.serialization; import java.io.IOException; -public interface PClassSerializer { - public byte[] serialize(X val) throws IOException; - public X deserialize(byte[] bytes) throws IOException; +public interface InstanceSerializer { + byte[] serialize(T instance) throws IOException; --- End diff -- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52951541 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java --- @@ -0,0 +1,22 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.coord.store; + +public interface TransientStoreListener { + void onChange(TransientStoreEvent event); --- End diff -- I typically prefer this way as it allows extending capabilities(esp event types) via polymorphism without requiring to alter the interface. I am fine with either way though. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52951273 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/PathUtils.java --- @@ -0,0 +1,59 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.coord.zk; + +import com.google.common.base.Preconditions; +import org.apache.parquet.Strings; + +public final class PathUtils { --- End diff -- done. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52950999 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/coord/ClusterCoordinator.java --- @@ -60,16 +62,23 @@ public abstract DistributedSemaphore getSemaphore(String name, int maximumLeases); /** + * Returns a new {@link TransientStore store} instance with the given {@link TransientStoreConfig configuration}. + * @param config store configuration + * @param value type for this store + */ + public abstract TransientStore newTransientStore(TransientStoreConfig config); + + /** * Actions to take when there are a set of new de-active drillbits. * @param unregisteredBits */ - public void drillbitUnregistered(Set unregisteredBits) { + protected void drillbitUnregistered(Set unregisteredBits) { --- End diff -- Wrong use of this method outside of subclasses could easily mess the system up. To my understanding, this method should only be visible to implementor who is the ultimate authority to fire listeners on topology changes, so protected seems to make more sense here. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52950554 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/serialization/JacksonSerializer.java --- @@ -0,0 +1,59 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.serialization; + +import java.io.IOException; +import java.util.Objects; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.ObjectReader; +import com.fasterxml.jackson.databind.ObjectWriter; + +public class JacksonSerializer implements InstanceSerializer { + private final ObjectReader reader; + private final ObjectWriter writer; + + public JacksonSerializer(final ObjectMapper mapper, final Class klazz) { +this.reader = mapper.reader(klazz); +this.writer = mapper.writer(); + } + + @Override + public T deserialize(final byte[] raw) throws IOException { +return reader.readValue(raw); + } + + @Override + public byte[] serialize(final T instance) throws IOException { +return writer.writeValueAsBytes(instance); + } + + @Override + public boolean equals(Object obj) { +if (obj instanceof JacksonSerializer && obj.getClass().equals(getClass())) { + final JacksonSerializer other = (JacksonSerializer)obj; + return Objects.equals(reader, other.reader) && Objects.equals(writer, other.writer); +} +return false; + } + + @Override + public int hashCode() { +return super.hashCode(); --- End diff -- nice catch. thanks. done. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
Github user hnfgns commented on a diff in the pull request: https://github.com/apache/drill/pull/374#discussion_r52950533 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZKClusterCoordinator.java --- @@ -206,6 +205,16 @@ public DistributedSemaphore getSemaphore(String name, int maximumLeases) { return new ZkDistributedSemaphore(curator, "/semaphore/" + name, maximumLeases); } + @Override + public TransientStore newTransientStore(final TransientStoreConfig config) { +final ZkEphemeralStore store = new ZkEphemeralStore<>(config, curator); --- End diff -- done. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4275: create TransientStore for short-li...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Resolved] (DRILL-3547) IndexOutOfBoundsException on directory with ~20 subdirectories
[ https://issues.apache.org/jira/browse/DRILL-3547?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Arina Ielchiieva resolved DRILL-3547. - Resolution: Cannot Reproduce > IndexOutOfBoundsException on directory with ~20 subdirectories > -- > > Key: DRILL-3547 > URL: https://issues.apache.org/jira/browse/DRILL-3547 > Project: Apache Drill > Issue Type: Bug > Components: Functions - Drill >Affects Versions: 1.1.0 > Environment: RHEL 7 >Reporter: Philip Deegan >Assignee: Arina Ielchiieva > Fix For: 1.6.0 > > > Tested on 1.1 with commit id: > {noformat} > 0: jdbc:drill:zk=local> select commit_id from sys.version; > +---+ > | commit_id | > +---+ > | e3fc7e97bfe712dc09d43a8a055a5135c96b7344 | > +---+ > {noformat} > Directory has child directories "a" to "u", each contain json files. > Running the query on each subdirectory indivudually does not cause an error. > {noformat} > java.lang.RuntimeException: java.sql.SQLException: SYSTEM ERROR: > IndexOutOfBoundsException: index: 0, length: 1 (expected: range(0, 0)) > Fragment 1:2 > [Error Id: 69a0879f-f718-4930-ae6f-c526de05528c on > ip-172-31-29-60.eu-central-1.compute.internal:31010] > at sqlline.IncrementalRows.hasNext(IncrementalRows.java:73) > at > sqlline.TableOutputFormat$ResizingRowsProvider.next(TableOutputFormat.java:87) > at sqlline.TableOutputFormat.print(TableOutputFormat.java:118) > at sqlline.SqlLine.print(SqlLine.java:1583) > at sqlline.Commands.execute(Commands.java:852) > at sqlline.Commands.sql(Commands.java:751) > at sqlline.SqlLine.dispatch(SqlLine.java:738) > at sqlline.SqlLine.begin(SqlLine.java:612) > at sqlline.SqlLine.start(SqlLine.java:366) > at sqlline.SqlLine.main(SqlLine.java:259) > {noformat} -- This message was sent by Atlassian JIRA (v6.3.4#6332)