[GitHub] [carbondata] jackylk commented on a change in pull request #2465: [CARBONDATA-2863] Refactored CarbonFile interface

[GitBox]

jackylk commented on a change in pull request #2465: [CARBONDATA-2863] Refactored CarbonFile interface
URL: https://github.com/apache/carbondata/pull/2465#discussion_r349920438
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AbstractDFSCarbonFile.java
 ##########
 @@ -58,85 +59,82 @@
    */
   private static final Logger LOGGER =
       LogServiceFactory.getLogService(AbstractDFSCarbonFile.class.getName());
-  protected FileStatus fileStatus;
-  public FileSystem fs;
+  protected FileSystem fileSystem;
   protected Configuration hadoopConf;
+  protected Path path;
 
-  public AbstractDFSCarbonFile(String filePath) {
+  AbstractDFSCarbonFile(String filePath) {
     this(filePath, FileFactory.getConfiguration());
   }
 
-  public AbstractDFSCarbonFile(String filePath, Configuration hadoopConf) {
-    this.hadoopConf = hadoopConf;
-    filePath = filePath.replace("\\", "/");
-    Path path = new Path(filePath);
-    try {
-      fs = path.getFileSystem(this.hadoopConf);
-      fileStatus = fs.getFileStatus(path);
-    } catch (IOException e) {
-      LOGGER.debug("Exception occurred:" + e.getMessage(), e);
-    }
+  AbstractDFSCarbonFile(String filePath, Configuration hadoopConf) {
+    this(new Path(filePath), hadoopConf);
   }
 
-  public AbstractDFSCarbonFile(Path path) {
+  AbstractDFSCarbonFile(Path path) {
     this(path, FileFactory.getConfiguration());
   }
 
-  public AbstractDFSCarbonFile(Path path, Configuration hadoopConf) {
+  AbstractDFSCarbonFile(Path path, Configuration hadoopConf) {
     this.hadoopConf = hadoopConf;
-    FileSystem fs;
     try {
-      fs = path.getFileSystem(this.hadoopConf);
-      fileStatus = fs.getFileStatus(path);
+      Path newPath = new Path(path.toString().replace("\\", "/"));
+      fileSystem = newPath.getFileSystem(this.hadoopConf);
+      this.path = newPath;
     } catch (IOException e) {
-      LOGGER.debug("Exception occurred:" + e.getMessage(), e);
+      throw new CarbonFileException("Error while getting File System: ", e);
     }
   }
 
-  public AbstractDFSCarbonFile(FileStatus fileStatus) {
-    this.hadoopConf = FileFactory.getConfiguration();
-    this.fileStatus = fileStatus;
+  AbstractDFSCarbonFile(FileStatus fileStatus) {
+    this(fileStatus.getPath());
   }
 
   @Override
   public boolean createNewFile() {
-    Path path = fileStatus.getPath();
-    FileSystem fs;
     try {
-      fs = fileStatus.getPath().getFileSystem(hadoopConf);
-      return fs.createNewFile(path);
+      return fileSystem.createNewFile(path);
     } catch (IOException e) {
-      return false;
+      throw new CarbonFileException("Unable to create file: " + path.toString(), e);
     }
   }
 
   @Override
   public String getAbsolutePath() {
-    return fileStatus.getPath().toString();
+    try {
+      return fileSystem.getFileStatus(path).getPath().toString();
+    } catch (IOException e) {
+      throw new CarbonFileException("Unable to get file status: ", e);
+    }
   }
 
   @Override
   public String getName() {
-    return fileStatus.getPath().getName();
+    return path.getName();
   }
 
   @Override
   public boolean isDirectory() {
-    return fileStatus.isDirectory();
+    try {
+      return fileSystem.getFileStatus(path).isDirectory();
 
 Review comment:
   Why remove the fileStatus member? Here it will call getFileStatus multiple times

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services