You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by vrozov <gi...@git.apache.org> on 2018/03/13 21:34:24 UTC
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
GitHub user vrozov opened a pull request:
https://github.com/apache/drill/pull/1163
DRILL-6053 & DRILL-6237
- Avoid excessive locking in LocalPersistentStore
- Upgrade checkstyle version to 5.9 or above
@arina-ielchiieva Please review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vrozov/drill DRILL-6237
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1163.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1163
----
commit adc5aea6ef217c3a3548e630b466f9b56adfb282
Author: Vlad Rozov <vr...@...>
Date: 2018-03-13T17:56:52Z
DRILL-6053: Avoid excessive locking in LocalPersistentStore
commit c1def24c62ccb729e720b5416abdb41c47a4869f
Author: Vlad Rozov <vr...@...>
Date: 2018-03-13T19:24:48Z
DRILL-6237: Upgrade checkstyle version to 5.9 or above
commit 6bde4215e8eb2bc19a1e981c3e444b43b08237ee
Author: Vlad Rozov <vr...@...>
Date: 2018-03-13T20:48:35Z
DRILL-6053: Avoid excessive locking in LocalPersistentStore
----
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174433648
--- Diff: common/src/main/java/org/apache/drill/common/AutoCloseables.java ---
@@ -25,6 +25,11 @@
*/
public class AutoCloseables {
+ public interface Closeable extends AutoCloseable {
--- End diff --
Not sure about benefit of this change... Could you please explain.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174530802
--- Diff: common/src/main/java/org/apache/drill/common/AutoCloseables.java ---
@@ -25,6 +25,11 @@
*/
public class AutoCloseables {
+ public interface Closeable extends AutoCloseable {
--- End diff --
The change allows avoiding `catch` in try-with-resources when `close()` does not throw an exception.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174533520
--- Diff: common/src/main/java/org/apache/drill/common/AutoCloseables.java ---
@@ -25,6 +25,11 @@
*/
public class AutoCloseables {
+ public interface Closeable extends AutoCloseable {
--- End diff --
But java has already `Closeable` interface, that's confusing having custom one with the same name.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174551433
--- Diff: common/src/main/java/org/apache/drill/common/AutoCloseables.java ---
@@ -25,6 +25,11 @@
*/
public class AutoCloseables {
+ public interface Closeable extends AutoCloseable {
--- End diff --
It is on purpose:
- There is a minimal difference between Drill `Closeable` and Java 'Closeable/AutoCloseable`, so name reflects that.
- It won't be possible to use Drill `Closeable` in place of Java `Closeable/AutoCloseable` in case `close()` throws checked exception.
- Drill `Closeable` is not a top level interface, so where it is necessary to distinguish Drill `Closeable` from Java `Closeable` full name `AutoClosables.Closeable` should be used.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174534739
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/Store.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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;
+
+public interface Store<V> extends AutoCloseable
+{
--- End diff --
Will do. What is the reason it is not handled by checkstyle?
---
[GitHub] drill issue #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1163
@vrozov, please address code review comments so we can merge the Jira.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174433799
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/PersistentStoreProvider.java ---
@@ -33,7 +34,9 @@
* @param <V> store value type
*/
<V> PersistentStore<V> getOrCreateStore(PersistentStoreConfig<V> config) throws StoreException;
-
+ default <V> VersionedPersistentStore<V> getOrCreateVersionedStore(PersistentStoreConfig<V> config) throws StoreException {
--- End diff --
Please add javadoc here and in newly created classes as well.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174431372
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/Store.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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;
+
+public interface Store<V> extends AutoCloseable
+{
--- End diff --
Please move to upper line.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174533741
--- Diff: pom.xml ---
@@ -259,7 +259,13 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>2.12.1</version>
- <configuration>
+ <dependencies>
+ <dependency>
+ <groupId>com.puppycrawl.tools</groupId>
+ <artifactId>checkstyle</artifactId>
+ <version>5.9</version>
+ </dependency>
+ </dependencies> <configuration>
--- End diff --
1. thanks for catching
2. each version of `maven-checkstyle-plugin` is preconfigured with a specific version of checkstyle (2.12.1 uses 5.7) that can be overridden by specifying that dependency explicitly.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174431507
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/VersionedPersistentStore.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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 org.apache.drill.exec.store.sys.store.DataChangeVersion;
+
+public interface VersionedPersistentStore<V> extends Store<V>
+{
--- End diff --
Please move to upper line.
---
[GitHub] drill issue #1163: DRILL-6053 & DRILL-6237
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1163
@arina-ielchiieva Addressed review comments. Please keep both commits (do not squash) during the merge.
---
[GitHub] drill issue #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1163
+1
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1163
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174433987
--- Diff: pom.xml ---
@@ -259,7 +259,13 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>2.12.1</version>
- <configuration>
+ <dependencies>
+ <dependency>
+ <groupId>com.puppycrawl.tools</groupId>
+ <artifactId>checkstyle</artifactId>
+ <version>5.9</version>
+ </dependency>
+ </dependencies> <configuration>
--- End diff --
1. Please `configuration` move to the next line.
2. You indicating that we upgrade dependency but in fact we added new?
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174541733
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/Store.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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;
+
+public interface Store<V> extends AutoCloseable
+{
--- End diff --
Well, we definitely need to improve Drill checkstyle. Tim used to bring up this topic on dev mailing list. For instance, there are many things we need to check, like javadocs for headers etc.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1163#discussion_r174432531
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/VersionedDelegatingStore.java ---
@@ -0,0 +1,118 @@
+/*
+ * 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.util.Iterator;
+import java.util.Map;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import org.apache.drill.common.AutoCloseables.Closeable;
+import org.apache.drill.common.concurrent.AutoCloseableLock;
+import org.apache.drill.exec.exception.VersionMismatchException;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreMode;
+import org.apache.drill.exec.store.sys.VersionedPersistentStore;
+
+public class VersionedDelegatingStore<V> implements VersionedPersistentStore<V>
+{
+ private final PersistentStore<V> store;
+ private final ReadWriteLock readWriteLock;
+ private final AutoCloseableLock readLock;
+ private final AutoCloseableLock writeLock;
+ private int version;
+
+ public VersionedDelegatingStore(PersistentStore<V> store) {
+ this.store = store;
+ readWriteLock = new ReentrantReadWriteLock();
+ readLock = new AutoCloseableLock(readWriteLock.readLock());
+ writeLock = new AutoCloseableLock(readWriteLock.writeLock());
+ version = -1;
+ }
+
+ @Override
+ public PersistentStoreMode getMode()
+ {
--- End diff --
Please move to the upper line.
---