okumin commented on code in PR #5776:
URL: https://github.com/apache/hive/pull/5776#discussion_r2052163988
##########
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java:
##########
@@ -1334,10 +1331,19 @@ public static class HdfsEncryptionShim implements
HadoopShims.HdfsEncryptionShim
private final Configuration conf;
- public HdfsEncryptionShim(URI uri, Configuration conf) throws IOException {
+ public static HdfsEncryptionShim createInstance(URI uri, Configuration
conf) throws IOException {
+ HdfsAdmin hadmin = null;
+ KeyProvider keyP = null;
Review Comment:
I think we don't need to initialize it as null.
##########
shims/0.23/src/main/java/org/apache/hadoop/mapred/WebHCatJTShim23.java:
##########
@@ -37,10 +37,7 @@
import java.io.IOException;
import java.net.URI;
import java.security.PrivilegedExceptionAction;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Set;
+import java.util.*;
Review Comment:
If I remember correctly, we don't use a wildcard import.
https://github.com/apache/hive/blob/be59bd0b445096e431c18bbfa0d682c78ba18b76/checkstyle/checkstyle.xml#L152-L155
You can run checkstyle with `mvn checkstyle:checkstyle -DskipTests -pl
shims/0.23`, and see the result in the target directory. It is unrealistic to
fix all issues, but it would be great if we can keep the codestyle consistent
as much as possible especially for new changes.
```
cat shims/0.23/target/checkstyle-result.xml | grep -A 1 WebHCatJTShim23
<file
name="/Users/shohei.okumiya/ghq/github.com/okumin/hive/shims/0.23/src/main/java/org/apache/hadoop/mapred/WebHCatJTShim23.java">
<error line="40" severity="warning" message="Using the '.*' form
of import should be avoided - java.util.*."
source="com.puppycrawl.tools.checkstyle.checks.imports.AvoidStarImportCheck"/>
```
##########
shims/common/pom.xml:
##########
@@ -106,6 +106,10 @@
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.hive</groupId>
+ <artifactId>hive-storage-api</artifactId>
+ </dependency>
Review Comment:
That makes sense... Can we move this class to shims-common so that both
shims-common and 0.23 can share it? Adding the whole dependency just for the
annotation might confuse maintainers or the chaos might cause a future problem.
https://github.com/apache/hive/blob/master/shims/0.23/src/main/java/org/apache/hadoop/util/SuppressFBWarnings.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
