DRILL-6053 & DRILL-6237#1163
Conversation
arina-ielchiieva
left a comment
There was a problem hiding this comment.
Vlad, I really like the changes, thanks for making them.
Please address some minor points though.
| import java.util.Map; | ||
|
|
||
| public interface Store<V> extends AutoCloseable | ||
| { |
There was a problem hiding this comment.
Please move to upper line.
There was a problem hiding this comment.
Will do. What is the reason it is not handled by checkstyle?
There was a problem hiding this comment.
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.
| import org.apache.drill.exec.store.sys.store.DataChangeVersion; | ||
|
|
||
| public interface VersionedPersistentStore<V> extends Store<V> | ||
| { |
There was a problem hiding this comment.
Please move to upper line.
|
|
||
| @Override | ||
| public PersistentStoreMode getMode() | ||
| { |
There was a problem hiding this comment.
Please move to the upper line.
| */ | ||
| public class AutoCloseables { | ||
|
|
||
| public interface Closeable extends AutoCloseable { |
There was a problem hiding this comment.
Not sure about benefit of this change... Could you please explain.
There was a problem hiding this comment.
The change allows avoiding catch in try-with-resources when close() does not throw an exception.
There was a problem hiding this comment.
But java has already Closeable interface, that's confusing to have custom one with the same name.
There was a problem hiding this comment.
It is intentional:
- There is a minimal difference between Drill
Closeableand Java 'Closeable/AutoCloseable`, so name reflects that. - It won't be possible to use Drill
Closeablein place of JavaCloseable/AutoCloseablein caseclose()throws checked exception. - Drill
Closeableis not a top level interface, so where it is necessary to distinguish DrillCloseablefrom JavaCloseablefull nameAutoClosables.Closeableshould be used.
| */ | ||
| <V> PersistentStore<V> getOrCreateStore(PersistentStoreConfig<V> config) throws StoreException; | ||
|
|
||
| default <V> VersionedPersistentStore<V> getOrCreateVersionedStore(PersistentStoreConfig<V> config) throws StoreException { |
There was a problem hiding this comment.
Please add javadoc here and in newly created classes as well.
pom.xml
Outdated
| <artifactId>checkstyle</artifactId> | ||
| <version>5.9</version> | ||
| </dependency> | ||
| </dependencies> <configuration> |
There was a problem hiding this comment.
- Please move
configurationto the next line. - You indicated that we upgrade dependency but in fact we add new?
There was a problem hiding this comment.
- thanks for catching
- each version of
maven-checkstyle-pluginis preconfigured with a specific version of checkstyle (2.12.1 uses 5.7) that can be overridden by specifying that dependency explicitly.
|
@vrozov, please address code review comments so we can merge the chnages. |
|
@arina-ielchiieva Addressed review comments. Please keep both commits (do not squash) during the merge. |
|
+1 |
@arina-ielchiieva Please review