-
Notifications
You must be signed in to change notification settings - Fork 374
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
[CELEBORN-1081][FOLLOWUP] Remove UNKNOWN_DISK and allocate all slots to disk #2098
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2098 +/- ##
==========================================
+ Coverage 46.50% 46.54% +0.04%
==========================================
Files 166 166
Lines 10758 10781 +23
Branches 984 988 +4
==========================================
+ Hits 5002 5017 +15
- Misses 5430 5436 +6
- Partials 326 328 +2 ☔ View full report in Codecov by Sentry. |
…to disk Signed-off-by: mingji <[email protected]>
@RexXiong @waitinfuture Take a look at this PR when you have time. |
diskIndex = (diskIndex + 1) % usableDiskInfos.size(); | ||
} | ||
usableDiskInfos.get(diskIndex).usableSlots--; | ||
DiskInfo selectedDiskInfo = usableDiskInfos.get(diskIndex).diskInfo; |
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.
IMO, we can put storage type in diskInfo, then we can directly know which type the disk is.
storageInfo = new StorageInfo(diskInfos[diskIndex].mountPoint(), availableStorageTypes); | ||
diskIndex = (diskIndex + 1) % diskInfos.length; | ||
} else { | ||
storageInfo = new StorageInfo(StorageInfo.Type.HDFS, availableStorageTypes); |
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.
why use HDFS directly at this branch?
@@ -218,7 +244,8 @@ private static List<Integer> roundRobin( | |||
boolean shouldReplicate, | |||
boolean shouldRackAware, | |||
int availableStorageTypes) { | |||
// workerInfo -> (diskIndexForPrimary, diskIndexForReplica) | |||
// workerInfo -> (diskIndexForPrimary, diskIndexForRe |
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.
link was break
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, merge to main(0.4.0)
What changes were proposed in this pull request?
Why are the changes needed?
To support the application's config about available storage types.
Does this PR introduce any user-facing change?
no.
How was this patch tested?
GA and Cluster.