kunal642 commented on a change in pull request #2465: [CARBONDATA-2863]
Refactored CarbonFile interface
URL: https://github.com/apache/carbondata/pull/2465#discussion_r350853582
##########
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:
fileStatus should not be a member variable because if the same carbonfile
object is used to check for exists after the file is deleted by another client
then the existing file status would return true instead of false.
There are many such methods in filestatus which can give false information
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
