venice icon indicating copy to clipboard operation
venice copied to clipboard

[server][dvc] Improve ST logging and add test coverage for endPosition subscription

Open sushantmane opened this issue 1 month ago • 0 comments

[server][dvc] Improve ST logging and add test coverage for endPosition subscription

Enhances server-side ST logging by including store version status and more descriptive replica-role details. Refactors store name and version parsing to avoid repeated extraction and centralizes store-type and version-role logic for clarity and reuse. Adds tests to verify the contract for endPosition subscription in PubSubConsumerAdapter.

AI Generated Summary Of Changes

Refactoring and code reuse:

  • The constructor of AbstractPartitionStateModel now parses and stores storeName and versionNumber from the resourceName once, making these values directly accessible throughout the class and reducing repeated parsing. Related getter methods (getStoreName, getVersionNumber) are added. [1] [2] [3]
  • Logic for determining the store type (VeniceUserStoreType) and the version role (e.g., CURRENT, BACKUP, FUTURE) is centralized in new or refactored helper methods (determineStoreType, getStoreType, getStoreVersionRole). This improves code maintainability and makes these utilities available for logging and other purposes.

Logging improvements:

  • Log messages for state transitions now include detailed replica type descriptions (e.g., "Hybrid store current version"), making logs more informative for debugging and operations. [1] [2] [3]

Simplification and consistency:

  • All uses of store name and version number in LeaderFollowerPartitionStateModel are updated to use the new getter methods, reducing code duplication and risk of parsing errors. [1] [2] [3] [4]
  • The store type determination logic is moved from inline code to a dedicated method, and the previous implementation is removed for clarity. [1] [2]

Test improvements:

  • The test setup in AbstractVenicePartitionStateModelTest is updated to use the new utility methods and to improve mock consistency, making tests clearer and more robust. [1] [2] [3] [4] [5]

Code changes

  • [ ] Added new code behind a config. If so list the config names and their default values in the PR description.
  • [ ] Introduced new log lines.
    • [ ] Confirmed if logs need to be rate limited to avoid excessive logging.

Concurrency-Specific Checks

Both reviewer and PR author to verify

  • [ ] Code has no race conditions or thread safety issues.
  • [ ] Proper synchronization mechanisms (e.g., synchronized, RWLock) are used where needed.
  • [ ] No blocking calls inside critical sections that could lead to deadlocks or performance degradation.
  • [ ] Verified thread-safe collections are used (e.g., ConcurrentHashMap, CopyOnWriteArrayList).
  • [ ] Validated proper exception handling in multi-threaded code to avoid silent thread termination.

How was this PR tested?

  • [ ] New unit tests added.
  • [ ] New integration tests added.
  • [ ] Modified or extended existing tests.
  • [ ] Verified backward compatibility (if applicable).

Does this PR introduce any user-facing or breaking changes?

  • [ ] No. You can skip the rest of this section.
  • [ ] Yes. Clearly explain the behavior change and its impact.

sushantmane avatar Nov 15 '25 09:11 sushantmane