Skip to content

Conversation

@mingri31164
Copy link
Contributor

@mingri31164 mingri31164 commented Sep 30, 2025

Problem

  1. Protocol rigidity: Field renaming causes continuous invalid refreshes due to MD5 mismatch
  2. Poor extensibility: Adding extended parameters triggers unnecessary client refreshes

Solution

  1. Multi-version protocol support (v1=full MD5, v2+=core MD5)
  2. Segment-level comparison algorithm: separate core and extended parameters
  3. Field adapter pattern for backward compatibility (coreSize/corePoolSize)
  4. Version-aware MD5 comparison on server side

Key Changes

  1. Add IncrementalContentUtil for core content extraction
  2. Add IncrementalMd5Util for versioned MD5 generation
  3. Add field adapters in ThreadPoolParameterInfo (corePoolSizeAdapt(), maximumPoolSizeAdapt())
  4. Update ConfigCacheService for version-aware comparison
  5. Client sends protocol version via X-Hippo4j-Protocol-Version header

Test Coverage

  1. ProtocolRigidityTest: 5 scenarios for field renaming compatibility
  2. ExtensibilityTest: 6 scenarios for extended parameter addition
  3. ContentUtilTest: 4 scenarios for field adapter verification

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements an incremental update protocol with multi-version compatibility to address protocol rigidity and extensibility issues. The solution separates core thread pool parameters from extended monitoring/alarm parameters, preventing unnecessary client refreshes when non-critical fields change or are renamed.

Key Changes:

  • Introduces protocol version 2 with core-only MD5 comparison (v1 remains full MD5 for backward compatibility)
  • Implements field adapters in ThreadPoolParameterInfo to handle coreSize/corePoolSize naming variations
  • Adds utilities for generating versioned MD5 hashes and extracting core vs. extended parameters

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
Md5ConfigUtil.java Extracts client protocol version from request header and passes to comparison logic
ConfigCacheService.java Adds version-aware MD5 comparison using incremental MD5 for v2+ clients
ClientWorker.java Updates client to use protocol version 2 and incremental content generation
CacheData.java Initializes cache with incremental content based on protocol version
IncrementalMd5Util.java Provides versioned MD5 generation and change type detection utilities
IncrementalContentUtil.java Implements core/extended parameter separation and field adapter integration
ContentUtil.java Integrates field adapters for backward-compatible content generation
ProtocolRigidityTest.java Tests field renaming compatibility across protocol versions
ExtensibilityTest.java Verifies extended parameter additions don't trigger unnecessary refreshes
ContentUtilTest.java Validates field adapter behavior with old and new field names

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@magestacks
Copy link
Member

@mingri31164 Please describe your testing process for the submitted code.

Also, does your submitted code cover all scenarios? For example, does it support smooth compatibility between different versions such as server 2.0.0 and client 1.5.0?

@mingri31164
Copy link
Contributor Author

mingri31164 commented Oct 28, 2025

Hi @magestacks , thank you for the review! Here's a comprehensive overview of my testing approach:

1️⃣ Unit Testing

1. ProtocolRigidityTest (5 test cases)

Addresses the field renaming issue:

Scenario Result
Old client (coreSize/maxSize) + New server (corePoolSize/maximumPoolSize) ✅ MD5 identical, no refresh
Field adapter priority (new fields over old) ✅ Correct priority
Field adapter fallback (old fields when new missing) ✅ Correct fallback
v1 vs v2 protocol difference ✅ Different strategies

2. ExtensibilityTest (6 test cases)

Addresses the extended parameter issue:

Scenario Result
Server adds executeTimeOut extended parameter ✅ v2 client MD5 unchanged
Server adds multiple extended parameters ✅ v2 client MD5 unchanged
Only extended params changed hasCoreChanges=false
v1 vs v2 behavior on extended param changes ✅ v1 refreshes, v2 doesn't

3. ContentUtilTest (4 test cases)

Validates content generation and field adapters:

Scenario Result
Generate correct thread pool content JSON ✅ All required fields included
Field adapter: old/new field names generate identical content Key backward compatibility test
GroupKey generation correctness ✅ Correct format

4. IncrementalMd5UtilBoundaryTest (10 test cases)

Boundary conditions and exception handling:

Scenario Result
null config parameter ✅ Correctly handles NPE
All null fields config ✅ Generates valid MD5
Version 0, negative, very large values ✅ Correct fallback logic
Config with only extended parameters ✅ v2 correctly excludes
Empty string vs null ✅ Correctly differentiates
MD5 calculation consistency ✅ Multiple calls return same result

5. Server Module Tests (in threadpool/server/config)

Md5ConfigUtilVersionTest (8 test cases)

  • ✅ Client sends v2 version header - Server correctly identifies
  • ✅ Old client without version header - Defaults to v1 (backward compatible)
  • ✅ Version header is empty string - Defaults to v1
  • ✅ Version header has invalid format - Graceful handling, defaults to v1
  • ✅ Version header explicitly v1 - Correctly identified
  • ✅ Version header is future version (v3) - Correctly parsed
  • ✅ v1 client version detection - compareMd5 will use v1 protocol
  • ✅ v2 client version detection - compareMd5 will use v2 protocol

ConfigCacheServiceVersionTest (4 test cases)

  • ✅ v1 and v2 produce different MD5 (with extended params)
  • ✅ v2 client - Extended param change doesn't trigger refresh
  • ✅ v1 client - Extended param change triggers refresh (backward compatible)
  • ✅ Both versions - Core param change triggers refresh

Note: Server tests focus on version detection logic and MD5 calculation strategy unit tests. Full ConfigCacheService integration tests (requiring configuration cache infrastructure) are part of system integration testing. This PR focuses on core protocol logic unit test coverage,and this part is currently being actively followed up for testing.

6. CacheDataContentDeliveryTest (5 test cases)

Validates content delivery layer (fixes content truncation bug):

Scenario Result
setContent stores full configuration with MD5 separation ✅ Full content stored, MD5 on core fields
Listener receives complete configuration ✅ All extended params (executeTimeOut, isAlarm) present
MD5 only changes on core parameter change ✅ Extended param changes don't affect MD5 (v2)
Bug scenario: Incremental content vs full content Detects pre-fix bug, validates fix
End-to-end: ServerThreadPoolDynamicRefresh parsing ✅ Refresh receives all critical extended params

Critical Fix Validation: Test 4 demonstrates the bug where using IncrementalContentUtil.getIncrementalContent() for content delivery (pre-fix) causes extended parameters to be null. Post-fix uses ContentUtil.getPoolContent() to preserve all fields while MD5 calculation still uses incremental content internally.


2️⃣ Cross-Version Compatibility: Server 2.0.0 + Client 1.5.0

Compatibility Matrix

Server Client Protocol Result
2.0.0 (v2) 1.5.0 (v1) v1 (full MD5) Compatible
2.0.0 (v2) 2.0.0 (v2) v2 (incremental) ✅ Compatible
1.5.0 (v1) 2.0.0 (v2) v1 (full MD5) ✅ Compatible

How Server 2.0.0 Supports Client 1.5.0:

1. Client 1.5.0 behavior:

  • Sends request without X-Hippo4j-Protocol-Version header
  • Uses old field names (coreSize/maxSize)

2. Server 2.0.0 detection:

// Md5ConfigUtil.java
private static int getClientVersion(HttpServletRequest request) {
    String versionHeader = request.getHeader(PROTOCOL_VERSION_HEADER);
    return StringUtil.isNotEmpty(versionHeader) ? 
           Integer.parseInt(versionHeader) : 1;  // ← Defaults to v1
}

3. Server adaptation:

// IncrementalMd5Util.java
public static String getVersionedMd5(ThreadPoolParameterInfo config, int clientVersion) {
    if (clientVersion >= 2) {
        return getCoreMd5(config);      // v2 client: incremental
    }
    return getFullMd5(config);          // v1 client: full MD5 ✅
}

4. Field name compatibility:

// ThreadPoolParameterInfo.java - Field adapter
public Integer corePoolSizeAdapt() {
    return corePoolSize != null ? corePoolSize : coreSize;  // Unified handling
}

Result: Old v1 clients receive exactly the same behavior as before—zero breaking changes.


3️⃣ Implementation Summary

1. Core Components:

  1. IncrementalContentUtil - Defines PROTOCOL_VERSION=2, core vs extended parameters
  2. IncrementalMd5Util - Version-aware MD5 calculation
  3. Md5ConfigUtil - Client version extraction (defaults to 1)
  4. ConfigCacheService - Uses versioned MD5 comparison
  5. ClientWorker - Adds protocol version header
  6. ThreadPoolParameterInfo - Field adapters for compatibility

2. Core Parameters (MD5 included):
tenantId, itemId, tpId, corePoolSize, maximumPoolSize, queueType, capacity, keepAliveTime, rejectedType, allowCoreThreadTimeOut

3. Extended Parameters (MD5 excluded in v2):
executeTimeOut, isAlarm, capacityAlarm, livenessAlarm, activeAlarm, rejectedAlarm, alarmInterval


@Createsequence Createsequence removed their request for review October 31, 2025 01:51
@magestacks magestacks merged commit 565c3f7 into opengoofy:develop Oct 31, 2025
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants