在業務中臺，如何做代碼評審

圖片

作者：西部

部門：業務中臺 / 測試開發

1、業務背景

=========

業務方應用接入 BOS 需要依賴於 bos-sdk，應用集羣在啓動時通過 bos-sdk 將應用指定註解的組件進行收集，收集完成後保存在 DB 中，集羣中的每一臺機器在重啓時，需要保證入庫時只有一條請求的處理能夠正確入庫，以保證數據不會重複入庫以及數據插入衝突的情況，爲防止出現上述情況，項目中採用分佈式鎖，對此我們針對項目中分佈式鎖的邏輯，以及業務拿到鎖的實現進行了 CR，CR 的最佳指導我們採用結構化方式進行，分別從背景瞭解、業務場景、邏輯分析、異常分析、編程規範、非功能分析、可測性分析這幾個唯度進行 CR。

2、我們先看一下收集流程

3、分佈鎖的實現

3.1 鎖實現邏輯：

3.2 鎖實現時序調用：

3.3 鎖流程處理：

3.4 我們針對上述鎖的實現開始 CR：

a 背景瞭解：

要了解背景，仍需要從代碼中的註釋作爲入口：
第 2 行中文字描述 “競爭獲取分佈式鎖” 可能讓人有點困惑，什麼樣的場景需要競爭鎖，沒有描述清楚：
第 4 行 interval 的註釋 “鎖住時間” 是不是改爲鎖的失效時間，更能讓人理解：

b 鎖業務場景分析：

場景分析一：線程永遠拿不到鎖：代碼第 10 行，如果發生異常，則返 false, 這樣導致線程獲取不到鎖；
場景分析二：線程 1 獲取鎖後，業務邏輯未處理完，鎖失效，線程 2 可獲取鎖，引起數據入庫 (dumplicate) 衝

c 邏輯分析：

場景遺漏：

try ....catch 中的 try 對應的邏輯發生異常，catch 則會返回 false，線程就會一直拿不到鎖；

邏輯冗餘：

第 1416 行的邏輯是根據 lastReqTime 是不爲空或長度不爲 0，直接返回 true，往上追蹤 lastReqTime 的值是第 10 行賦值，假設緩存 readis 不存在巖機可服務不可用情況，也不考慮網絡抖動情況，在對 key 賦值是永運是成功的，即永遠有值，則第 1416 的條件邏輯是多餘的邏輯，即使出現巖機，網絡抖動情況則對 key 的賦值會失敗，走 catch 後，直接會返回 false；

d 非功能點分析

可測性分析：

無發現問題 (可以單測，將 StringRedisTemplate 寫成檔板)

可監控：

第 28 行，可以按關鍵字進行監控告警，但根據輸出的 ERROR 日誌對定位問題可能會有誤導，

4、業務獲鎖後處理邏輯

4.1 業務代碼邏輯處理:

4.2 業務獲取鎖業務處理流程:

4.3 問題分析：

a 背景瞭解 (註釋中相關業務場景信息缺失)：

業務執行邏輯根據是否獲取鎖，走不同的邏輯處理，在註釋中是瞭解不到的，此時只能通過業務的上下文去理解拿鎖後的邏輯，註釋欠缺；

b 邏輯分析

邏輯處理不合理：

第 31~36 行

try 語句塊的邏輯在此場景核心是關注 DB 操作的，不應在 try 語句塊中加入其它邏輯調用，換句話理解，如果 DB 操作成功，第 34 行調用失敗或調用異常，則會走 catch，與 try 中關心場景本意不符。

第 34 行服務調用是一個空的實現類，裏面沒有任何邏輯，此處放在這裏，用意不明確，建議刪除

c 代碼寫法規範：

第 13 行，return Boolean.TRUE 這種寫法是考慮性能上的差異嗎？return true,return Boolean.TRUE，如無區別，建議統一風格；
第 37 行，catch 中的異常拋出，建議加上 error 日誌的打印，具備監控預警能力；
註釋不規範第 19 行，鎖 sleep 3 秒，與第 21 代碼實現衝突；
Thread.sleep(4000) 爲什麼是 4S，而不是 5000s 或其它的數值，根據什麼依據設置的，沒有進行說明；
日誌級別打印級別不規範：第 33 行，log.info("MindSaveConsumer-process saveOrUpdateMindAggr {}", saveSuccess ? "success" : "fail"); 這行是根據 DB 操作後，返回不同結果的日誌打印，不管 DB 操作失敗還是成功，打印的日誌級別都是 info，理論上講對於 DB 操作異常的日誌都應打 error 級別；
第 26、27 行，log.info("MindSaveConsumer-process locked version is {}",key)<26 行 >；throw new AbilityException(ErrorTypeEnum.LOCK_FAIL)<27 號 >；
結合 27 行給拋出的異常，建議第 26 行打印日誌級別設置 error 或 waring 更合理，或者說線程本身在獲取鎖時因鎖被佔正常的行爲，則 27 行沒必要拋出異常，直接打印出 info 或 waring 更合理一點；且 27 行拋出異常描述爲 “加鎖失敗”，這個描述讓人誤解，建議更改爲鎖被佔，獲取鎖失敗，更適合此場景的異常描述；

異常分析：

線程永遠拿不到鎖：鎖實現的 10 行代碼出現異常，則返回 false，此時後續請求進來會永遠拿不到鎖；
拿鎖的線程執行邏輯時間較長超過 15s 失效釋放鎖，則線程 2 拿鎖之後，在進行 DB 操作時會產生 dumplicate 的衝突

d 非功能分析