[MINOR] fix: allow mountPoint not containing '/'
#607
Conversation
Codecov Report
@@ Coverage Diff @@
## master #607 +/- ##
============================================
+ Coverage 60.85% 60.88% +0.02%
- Complexity 1800 1802 +2
============================================
Files 214 214
Lines 12387 12388 +1
Branches 1044 1045 +1
============================================
+ Hits 7538 7542 +4
+ Misses 4444 4441 -3
Partials 405 405
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
storage/src/main/java/org/apache/uniffle/storage/common/DefaultStorageMediaProvider.java
Outdated
Show resolved
Hide resolved
storage/src/main/java/org/apache/uniffle/storage/common/DefaultStorageMediaProvider.java
Outdated
Show resolved
Hide resolved
| @@ -40,4 +40,11 @@ public void testStorageProvider() { | |||
| // invalid uri should also be reported as HDD | |||
| assertEquals(StorageMedia.HDD, provider.getStorageMediaFor("file@xx:///path/to/a")); | |||
| } | |||
|
|
|||
| @Test | |||
| public void getGetDeviceName() { | |||
There was a problem hiding this comment.
could you also add getStorageMediaFor for "rootfs" path and check its return media is HDD or not?
There was a problem hiding this comment.
I think it's not predictable. Depends on the CI environment.
There was a problem hiding this comment.
I don't know how to do it. I don't know which path will return rootfs in CI environment.
There was a problem hiding this comment.
how about mock the getDeviceName method?
Seems it was declared as static, maybe an instance method is easier to be mocked?
There was a problem hiding this comment.
how about mock the
getDeviceNamemethod?Seems it was declared as static, maybe an instance method is easier to be mocked?
I just tried, but failed. We need to specify the return value for getStorageMediaFor, otherwise null will be returned.
There was a problem hiding this comment.
that doesn't make sense...
I'm ok with current status. Let's defer it.
kaijchen
changed the title
mountPoint do not contains '/'mountPoint not containing '/'
What changes were proposed in this pull request?
Allow
mountPointdo not contains '/'Why are the changes needed?
mountPointis not always contains '/', such asrootfsDoes this PR introduce any user-facing change?
No
How was this patch tested?
No need.