okumin commented on code in PR #5776:
URL: https://github.com/apache/hive/pull/5776#discussion_r2051686241
##########
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java:
##########
@@ -774,26 +772,28 @@ public FutureDataInputStreamBuilder openFile(Path path)
throws IOException, Unsu
}
@Override
- public RemoteIterator<LocatedFileStatus> listLocatedStatus(final Path f)
- throws FileNotFoundException, IOException {
- return new RemoteIterator<LocatedFileStatus>() {
- private final RemoteIterator<LocatedFileStatus> stats =
- ProxyFileSystem23.super.listLocatedStatus(
- ProxyFileSystem23.super.swizzleParamPath(f));
-
- @Override
- public boolean hasNext() throws IOException {
- return stats.hasNext();
- }
+ public RemoteIterator<LocatedFileStatus> listLocatedStatus(final Path f)
throws FileNotFoundException, IOException {
+ try {
+ final RemoteIterator<LocatedFileStatus> remoteIterator =
+
ProxyFileSystem23.super.listLocatedStatus(ProxyFileSystem23.super.swizzleParamPath(f));
+ return new RemoteIterator<LocatedFileStatus>() {
+ private final RemoteIterator<LocatedFileStatus> stats =
remoteIterator;
+
+ @Override
+ public boolean hasNext() throws IOException {
+ return stats.hasNext();
+ }
- @Override
- public LocatedFileStatus next() throws IOException {
- LocatedFileStatus result = stats.next();
- return new LocatedFileStatus(
- ProxyFileSystem23.super.swizzleFileStatus(result, false),
- result.getBlockLocations());
- }
- };
+ @Override
+ public LocatedFileStatus next() throws IOException {
+ LocatedFileStatus result = stats.next();
+ return new
LocatedFileStatus(ProxyFileSystem23.super.swizzleFileStatus(result, false),
+ result.getBlockLocations());
+ }
+ };
+ } catch (IOException e) {
Review Comment:
Can we remove this try-catch because this method throws an IOException?
##########
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java:
##########
@@ -1334,10 +1335,23 @@ 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;
+ try {
+ hadmin = new HdfsAdmin(uri, conf);
+ keyP = hadmin.getKeyProvider();
+ } catch (IOException e) {
Review Comment:
Can we remove this try-catch?
##########
shims/common/src/main/java/org/apache/hadoop/hive/shims/ShimLoader.java:
##########
@@ -87,7 +87,9 @@ public static HadoopShims getHadoopShims() {
}
}
}
- return hadoopShims;
+ synchronized (ShimLoader.class) {
+ return loadShims(HADOOP_SHIM_CLASSES, HadoopShims.class);
+ }
Review Comment:
I think we intentionally return a cached `hadoopShims` for performance. I
guess we need to mark it as a false positive.
- https://github.com/spotbugs/spotbugs/issues/1747
- https://github.com/apache/hbase/pull/4787/files
##########
shims/common/src/main/java/org/apache/hadoop/fs/ProxyFileSystem.java:
##########
@@ -207,21 +199,25 @@ public FileStatus[] listStatus(Path f) throws IOException
{
@Override //ref. HADOOP-12502
public RemoteIterator<FileStatus> listStatusIterator(Path f) throws
IOException {
- return new RemoteIterator<FileStatus>() {
- private final RemoteIterator<FileStatus> orig =
- ProxyFileSystem.super.listStatusIterator(swizzleParamPath(f));
-
- @Override
- public boolean hasNext() throws IOException {
- return orig.hasNext();
- }
-
- @Override
- public FileStatus next() throws IOException {
- FileStatus ret = orig.next();
- return swizzleFileStatus(ret, false);
- }
- };
+ try {
+ final RemoteIterator<FileStatus> remoteIterator =
ProxyFileSystem.super.listStatusIterator(swizzleParamPath(f));
+ return new RemoteIterator<FileStatus>() {
+ private final RemoteIterator<FileStatus> orig = remoteIterator;
+
+ @Override
+ public boolean hasNext() throws IOException {
+ return orig.hasNext();
+ }
+
+ @Override
+ public FileStatus next() throws IOException {
+ FileStatus ret = orig.next();
+ return swizzleFileStatus(ret, false);
+ }
+ };
+ } catch (IOException e) {
Review Comment:
Can we remove this try-catch?
##########
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:
Do we need this one?
##########
shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShimsSecure.java:
##########
@@ -185,10 +185,27 @@ public float getProgress() throws IOException {
* A generic RecordReader that can hand out different recordReaders
* for each chunk in the CombineFileSplit.
*/
- public CombineFileRecordReader(JobConf job, CombineFileSplit split,
- Reporter reporter,
- Class<RecordReader<K, V>> rrClass)
- throws IOException {
+ public static <K,V> CombineFileRecordReader<K,V> createInstance(JobConf
job, CombineFileSplit split,
+ Reporter
reporter,
+
Class<RecordReader<K, V>> rrClass) throws IOException{
Review Comment:
```suggestion
Class<RecordReader<K, V>> rrClass) throws IOException {
```
##########
shims/common/src/main/java/org/apache/hadoop/hive/io/HdfsUtils.java:
##########
@@ -211,8 +210,20 @@ public HadoopFileStatus(Configuration conf, FileSystem fs,
Path file) throws IOE
this.aclStatus = aclStatus;
}
+ public static HadoopFileStatus createInstance (Configuration conf,
FileSystem fs, Path file) throws IOException {
+ FileStatus fileStatus = fs.getFileStatus(file);
+ return new HadoopFileStatus(conf, fs, fileStatus, file);
+ }
+
public FileStatus getFileStatus() {
- return fileStatus;
+ Path symlink;
Review Comment:
Thanks. In my current impression, it might be safer to keep using the
original status with an annotation. That's because `FileStatus` is heavily
extended by various classes, and it is not self-evident that we can clone it as
the parent class.
https://github.com/apache/hadoop/blob/rel/release-3.4.1/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsNamedFileStatus.java
##########
shims/0.23/src/main/java/org/apache/hadoop/mapred/WebHCatJTShim23.java:
##########
@@ -51,19 +48,36 @@ public class WebHCatJTShim23 implements WebHCatJTShim {
/**
* Create a connection to the Job Tracker.
*/
- public WebHCatJTShim23(final Configuration conf, final UserGroupInformation
ugi)
- throws IOException {
- try {
- this.conf = conf;
- jc = ugi.doAs(new PrivilegedExceptionAction<JobClient>() {
- public JobClient run() throws IOException, InterruptedException {
- //create this in doAs() so that it gets a security context based
passed in 'ugi'
- return new JobClient(conf);
- }
- });
+ public static WebHCatJTShim createInstance(final Configuration conf, final
UserGroupInformation ugi)
+ throws IOException {
+ int[] flag = new int[1];
+ ArrayList<String> eMessage = new ArrayList<>();
+ WebHCatJTShim webHCatJTShim = new WebHCatJTShim23(conf, ugi, flag,
eMessage);
+ if (flag[0] == -1) {
+ throw new IOException("Failed to create job Client" + eMessage.get(0));
+ } else if (flag[0] == -2) {
+ throw new RuntimeException("Failed to create job Client" +
eMessage.get(0));
}
- catch(InterruptedException ex) {
- throw new RuntimeException("Failed to create JobClient", ex);
+ return webHCatJTShim;
+ }
+
+ private WebHCatJTShim23(final Configuration conf, final UserGroupInformation
ugi, int[] flag,
+ ArrayList<String> eMessage) {
+ this.conf = new Configuration(conf);
+ try {
+ this.jc = ugi.doAs(new PrivilegedExceptionAction<JobClient>() {
+ public JobClient run() throws IOException, InterruptedException {
+ //create this in doAs() so that it gets a security context based
passed in 'ugi'
+ return new JobClient(conf);
+ }
+ });
+ } catch (InterruptedException ex) {
+ eMessage.add(ex.getMessage());
+ flag[0] = -2;
+ this.jc = null;
+ } catch (IOException ex) {
+ flag[0] = -1;
+ this.jc = null;
}
Review Comment:
Can we pull out the error-handling from the constructor like this one?
```
public static WebHCatJTShim createInstance(final Configuration conf, final
UserGroupInformation ugi)
throws IOException {
JobClient jc;
try {
jc = ugi.doAs((PrivilegedExceptionAction<JobClient>) () -> {
//create this in doAs() so that it gets a security context based
passed in 'ugi'
return new JobClient(conf);
});
} catch (InterruptedException e) {
throw new RuntimeException("Failed to create JobClient", e);
}
return new WebHCatJTShim23(conf, jc);
}
private WebHCatJTShim23(final Configuration conf, final JobClient jc) {
this.conf = new Configuration(conf);
this.jc = jc;
}
```
--
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
