Repository navigation
Refactor excessively long functions #1727
Description
Top 10 longest functions in BlazingMQ in number of lines (C++ only):
┌─────┬───────┬──────────────────────────────────────────────────┬────────────────────────────────────────────┐
│ # │ Lines │ Function │ Location │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 1 │ 1670 │ FileStore::recoverMessages │ mqbs/mqbs_filestore.cpp:1119 │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 2 │ 773 │ FileStore::openInRecoveryMode │ mqbs/mqbs_filestore.cpp:345 │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 3 │ 654 │ StorageUtil::recoveredQueuesCb │ mqbc/mqbc_storageutil.cpp:1555 │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 4 │ 612 │ RecoveryManager::processStorageSyncRequest │ mqbblp/mqbblp_recoverymanager.cpp:3718 │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 5 │ 474 │ RecoveryManager::onStorageSyncResponseDispatched │ mqbblp/mqbblp_recoverymanager.cpp:892 │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 6 │ 440 │ FileStore::writeJournalRecord │ mqbs/mqbs_filestore.cpp:4740 │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 7 │ 439 │ RecoveryManager::processPartitionSyncDataRequest │ mqbblp/mqbblp_recoverymanager.cpp:4483 │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 8 │ 383 │ RecoveryManager::openRecoveryFileSet │ mqbc/mqbc_recoverymanager.cpp:941 │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 9 │ 371 │ IncoreClusterStateLedger::applyImpl │ mqbc/mqbc_incoreclusterstateledger.cpp:900 │
├─────┼───────┼──────────────────────────────────────────────────┼────────────────────────────────────────────┤
│ 10 │ 325 │ FileStore::rolloverImpl │ mqbs/mqbs_filestore.cpp:2815 │
└─────┴───────┴──────────────────────────────────────────────────┴────────────────────────────────────────────┘
Long functions are likely overloaded and should be refactored into smaller ones.
Metadata
Metadata
Assignees
Labels
No labels
Activity
[-]Refactoring excessively long functions[/-][+]Refactor excessively long functions[/+]FulmonyX commentedon Sep 6, 2026
I'd like to work on this. Please assign it to me.
rmukondiwa commentedon Sep 10, 2026
Hi @kaikulimu , could I take #10 on this list,
FileStore::rolloverImpl(mqbs_filestore.cpp:2815)? Would like to be assigned if that's ok.I've been reading through the function and I think a good first step is just pulling out the parts that are pretty self-contained into private helpers, and leaving the trickier state-changing stuff (the first sync point write and the truncate/GC/file-set swap at the end) where it is. What I'm thinking:
logQueueRolloverSummary(...)for the queue summary table (around L2832-2863)logCompactionMetrics(...)for the new/old file size logging (around L2977-3014)copyOutstandingRecords(...)for the loop overd_recordsthat callswriteRolledOverRecord(around L2819-2830)The two logging ones only read state so I'd make them const. No behavior change, just moving cohesive blocks out, kind of like the recent
stopTransportAndClusters()split in mqba::Application. I'll check it against the existing FileStore tests since test8 already covers the rollover path, and the "no side-effect on failure" note in the header should still hold since the only early return is before anything gets mutated.Planning to keep it to just this one function for now. Does that seem like a reasonable approach, and is there anything about rolloverImpl I should watch out for?
kaikulimu commentedon Sep 12, 2026
@rmukondiwa I think your approach is good. Pure refactoring with no behavioral change is always a good first step. Thank you so much!
kaikulimu commentedon Sep 12, 2026
@FulmonyX Thanks so much for helping out. Let us know if you need any help. I am on vacation for the next two weeks, but @678098 and @dorjesinpo are good resources to reach out to if you have any questions.
kaikulimu commentedon Sep 12, 2026
@FulmonyX @rmukondiwa You two can coordinate on who is working on which items.
SrinathhSatuluri commentedon Oct 6, 2026
Hello @kaikulimu, could I take #6,
FileStore::writeJournalRecord? From my understanding, the function is a shared preamble and epilogue wrapped around four record-type branches, two of which carry most of the length. The natural split is to keep the shared parts in place and move the JOURNAL_OP and QUEUE_OP branches into their own private helpers, leaving the short CONFIRM and DELETION branches inline. We'd keep the validation and return codes exactly as they are to keep it as a pure refactor, and the existingFileStoretests and ITs cover the path. Does this sound right?kaikulimu commentedon Oct 9, 2026
@SrinathhSatuluri Yes, that makes sense for me. You can start working on it. Thanks!
FulmonyX commentedon Oct 11, 2026
Hi @kaikulimu and everyone,
I've now read through IncoreClusterStateLedger::applyImpl and the class header. It's ~371 lines and essentially a validation pipeline followed by a dispatch, so it's a good candidate to split. Here's my plan:
The function has these phases:
source validation
event header validation
record header validation
LSN + record type validation (leader vs follower branches)
cluster message loading
partition-primary advisory validation (~100 lines)
applyRecordInternal call
SNAPSHOT gated-update cleanup
For a first, purely mechanical PR, I'd like to extract just:
validateSource() (phase 1)
validateLsnAndRecordType() (phase 4)
and keep phases 2/3/5/6/7/8 inline. Phase 4 is self-contained and read-only; phase 6 involves the primary→replica "EXIT" path so I'd rather leave it for a follow-up PR to keep the diff safe.
One thing I want to confirm: the RcEnum error codes are currently declared inside applyImpl. For the extracted helpers I'd move them to a file-local enum (not touching the class public API) so the helpers can return the same codes. Does that sound reasonable?
Also, as I mentioned, I'm a student with limited bandwidth, so this will likely take me a few weeks, but I will get a commit in.
Translated by DeepSeek
FulmonyX commentedon Oct 11, 2026
Quick correction to my earlier message: phase 4 isn't strictly read-only — it has one side effect (d_gatedUpdateLsns.insert). It's still self-contained and safe to extract as a member helper; I just wanted to be precise.
kaikulimu commentedon Oct 13, 2026
@FulmonyX Extracting Phase 1 and Phase 4 makes sense. You can go ahead.
For returning error codes, I would not recommend creating file-local enum because it will pollute the file. I recommend one of the two ways:
falseinto the proper enum. This makes sense for Phase 1 as there is only one possible error code.ClusterStateLedgerUtilRc, and the parent function returnsrc * 100 + e_PARENT_ERROR_XXX. There are many examples in our codebase, for example here. Note that I just realize returningrc * 10in the example is a bug because the rcEnums forIncoreClusterStateLedger::applyImpgoup to -10, so I made Fix[mqb]: use 100x multiplier for rc enums reaching -10 #1814 to fix. This makes more sense for Phase 4.FulmonyX commentedon Oct 16, 2026
kaikulimu commentedon Oct 23, 2026
We are introducing Raft mode into our storage layer, per #1574 , and massive refactoring of our codebase would already be in place. Therefore, we are putting this issue on pause. We will resume this issue once we have more clarity on how the code would look like post-Raft.