Allowing consumer of HS specified device type for each service #355
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #355 +/- ##
==========================================
- Coverage 61.01% 58.00% -3.02%
==========================================
Files 136 108 -28
Lines 14598 9515 -5083
Branches 1756 1230 -526
==========================================
- Hits 8907 5519 -3388
+ Misses 4958 3459 -1499
+ Partials 733 537 -196 ☔ View full report in Codecov by Sentry. |
0470247 to
a7c3a97
Compare
Setting to auto will decide based on whether we have FAST and the service characteristic. Also chaning pct_to_size to take the size of that type. Signed-off-by: Xiaoxi Chen <xiaoxchen@ebay.com>
Signed-off-by: Xiaoxi Chen <xiaoxchen@ebay.com>
Signed-off-by: Xiaoxi Chen <xiaoxchen@ebay.com>
| @@ -365,7 +425,7 @@ shared< VirtualDev > HomeStore::create_vdev_cb(const vdev_info& vinfo, bool load | |||
| } | |||
|
|
|||
| uint64_t HomeStore::pct_to_size(float pct, HSDevType dev_type) const { | |||
| uint64_t sz = uint64_cast((pct * static_cast< double >(m_dev_mgr->total_capacity())) / 100); | |||
| uint64_t sz = uint64_cast((pct * static_cast< double >(m_dev_mgr->total_capacity(dev_type))) / 100); | |||
There was a problem hiding this comment.
This is the most significant behavior change in the case of hybrid storage.
Previously we calculate the size based on PCT * (HDD+SSD) , now it is PCT * size_of_target_device_type
conanfile.py
Outdated
| @@ -5,7 +5,7 @@ | |||
|
|
|||
| class HomestoreConan(ConanFile): | |||
| name = "homestore" | |||
| version = "6.0.1" | |||
| version = "6.1.1" | |||
There was a problem hiding this comment.
Technically a major (non-backwards compat) change. So if not going to bump Major, patch is good enough?
| @@ -87,7 +87,7 @@ constexpr uint16_t MAX_UUID_LEN{128}; | |||
| static constexpr hs_uuid_t INVALID_SYSTEM_UUID{0}; | |||
|
|
|||
| ///////////// All Enums ////////////////////////// | |||
| ENUM(HSDevType, uint8_t, Data, Fast); | |||
| ENUM(HSDevType, uint8_t, Auto, Data, Fast); | |||
There was a problem hiding this comment.
I guess I am not clear on how HomeObj will specify the percentage for each service with this Auto devType in place.
How can HomeObj know whether it is mixed dev type, so that META+LOG+INDEX can accumulate upto 100% of the SSD drive, and the replication could consume 100% of the other devType drives, and if it is not mixed, say all HDD, or all NVMe, then META+LOG+INDEX+REPLICATION should accumulate upto 100%.
Can you share how is HomeObject going to use this?
src/lib/homestore.cpp
Outdated
| // applying optimal placement for auto | ||
| for (auto& [svc_type, fparams] : format_opts) { | ||
| if ((svc_type & HS_SERVICE::META) && has_meta_service()) { | ||
| if (fparams.dev_type == HSDevType::Auto) { fparams.dev_type = get_optimal_dev_type(svc_type); } |
There was a problem hiding this comment.
To simplify the logic a bit, I am thinking can we relax this Auto check, and call get_optimal_dev_type directly for every services, the logic is consumer can specify FAST, but if fast is not available, we fall back to DATA.
This is the logic we apply for 1.3 HomeStore also. Does it make sense?
This doesn't resolve the percentage thing (mentioned eBay/HomeObject#110) for mixed/non-mixed drives type, though (which I am not clear with this PR -- my above comment).
Signed-off-by: Xiaoxi Chen <xiaoxchen@ebay.com>
|
LGTM, can you also run |
|
Merge as test_raft_repl_dev passsed |
This is the change we need for production , for more background we recorded here eBay/HomeObject#110
Consumer setting the device type to auto will let HS choose based on what it has.