-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#895] improvement: Rename Hdfs*.java to Hadoop*.java to support other Hadoop FS-compatible distributed filesystem #898
Conversation
…lder Signed-off-by: jiafu zhang <jiafu.zhang@intel.com>
HDFS(4), | ||
LOCALFILE_HDFS(6), | ||
HADOOP(4), | ||
LOCALFILE_HADOOP(6), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid modifying the config option? Because this is a incompatible change. Or we should make this change compatible. We should make old config option effective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid modifying the config option? Because this is a incompatible change. Or we should make this change compatible. We should make old config option effective.
make sense. Let me rollback that part.
…op FS-compatible distributed filesystem Signed-off-by: jiafu zhang <jiafu.zhang@intel.com>
…op FS-compatible distributed filesystem Signed-off-by: Jiafu Zhang <jiafu.zhang@intel.com>
…op FS-compatible distributed filesystem Signed-off-by: Jiafu Zhang <jiafu.zhang@intel.com>
…op FS-compatible distributed filesystem Signed-off-by: Jiafu Zhang <jiafu.zhang@intel.com>
Codecov Report
@@ Coverage Diff @@
## master #898 +/- ##
============================================
+ Coverage 55.22% 57.07% +1.84%
- Complexity 2197 2200 +3
============================================
Files 333 313 -20
Lines 16444 14089 -2355
Branches 1306 1307 +1
============================================
- Hits 9081 8041 -1040
+ Misses 6851 5607 -1244
+ Partials 512 441 -71
... and 27 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @jiafuzha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @jiafuzha
What changes were proposed in this pull request?
In server and storage modules, there are many classes prefixed with Hdfs which use Hadoop FS API and are thus impl agnostic, not depending on specific Hdfs impl. So, it's better to rename them to Hadoop* so that we can support other Hadoop FS compatible distributed filesystem by extending existing classes. It'll make code look more naturally.
There may be some slight differences among different Hadoop FS impls, like hadoop-daos not have a dedicated thread for reading and writing data, which is different from hdfs. Thus, we don't need to close outputstream at each flush to FS.
Why are the changes needed?
Fix: #895
Does this PR introduce any user-facing change?
No.
How was this patch tested?
CI passed.