[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
Github user asfgit closed the pull request at: https://github.com/apache/drill/pull/1163 ---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
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
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 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
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 extends AutoCloseable
+{
--- End diff --
Will do. What is the reason it is not handled by checkstyle?
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
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 @@ org.apache.maven.plugins maven-checkstyle-plugin 2.12.1 - + + +com.puppycrawl.tools +checkstyle +5.9 + + --- 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
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
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
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 extends AutoCloseable
+{
--- End diff --
Please move to upper line.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
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 @@ org.apache.maven.plugins maven-checkstyle-plugin 2.12.1 - + + +com.puppycrawl.tools +checkstyle +5.9 + + --- 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
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 extends Store
+{
--- End diff --
Please move to upper line.
---
[GitHub] drill pull request #1163: DRILL-6053 & DRILL-6237
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
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 store value type
*/
PersistentStore getOrCreateStore(PersistentStoreConfig config)
throws StoreException;
-
+ default VersionedPersistentStore
getOrCreateVersionedStore(PersistentStoreConfig 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
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 implements
VersionedPersistentStore
+{
+ private final PersistentStore store;
+ private final ReadWriteLock readWriteLock;
+ private final AutoCloseableLock readLock;
+ private final AutoCloseableLock writeLock;
+ private int version;
+
+ public VersionedDelegatingStore(PersistentStore 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.
---
[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 Date: 2018-03-13T17:56:52Z DRILL-6053: Avoid excessive locking in LocalPersistentStore commit c1def24c62ccb729e720b5416abdb41c47a4869f Author: Vlad Rozov Date: 2018-03-13T19:24:48Z DRILL-6237: Upgrade checkstyle version to 5.9 or above commit 6bde4215e8eb2bc19a1e981c3e444b43b08237ee Author: Vlad Rozov Date: 2018-03-13T20:48:35Z DRILL-6053: Avoid excessive locking in LocalPersistentStore ---
