Skip to content

Conversation

@sushantmane
Copy link
Contributor

@sushantmane sushantmane commented Nov 18, 2025

[server][dvc] Preserve PCS during non-Helix UNSUBSCRIBE to maintain quota accuracy

Keep PartitionConsumptionState entries in partitionConsumptionStateMap
for non-Helix UNSUBSCRIBE actions. Previously PCS was removed for all
UNSUBSCRIBE triggers, causing quota computation to lose visibility into
partitions unsubscribed due to errors or after end-of-push for batch stores.

PCS is now removed only for Helix-initiated or for DVC UNSUBSCRIBE, to
ensure storage utilization tracking and quota enforcement remain accurate.

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.

Copilot AI review requested due to automatic review settings November 18, 2025 07:22
Copilot finished reviewing on behalf of sushantmane November 18, 2025 07:26
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 fixes a quota tracking issue by preserving PartitionConsumptionState (PCS) entries during non-Helix UNSUBSCRIBE operations. Previously, PCS was removed for all unsubscribe triggers, causing the quota computation to lose visibility into partitions that were unsubscribed due to errors or after end-of-push for batch stores.

  • Moved partitionConsumptionStateMap.remove(partition) inside the Helix-triggered action conditional
  • PCS now only removed for Helix-initiated unsubscribe, ensuring storage utilization tracking and quota enforcement remain accurate
  • Aligns with existing pattern for failedPartitions and storageUtilizationManager cleanup

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sushantmane sushantmane enabled auto-merge (squash) November 22, 2025 19:56
Copilot AI review requested due to automatic review settings November 22, 2025 21:03
Copilot finished reviewing on behalf of sushantmane November 22, 2025 21:08
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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…on-Helix triggered actions

Remove PCS if it's dvc client

Fix flaky test

Fix another flaky test

Address review comments

Delete test that's already covered in other tests

Redistributed tests
Copilot AI review requested due to automatic review settings November 24, 2025 19:35
Copilot finished reviewing on behalf of sushantmane November 24, 2025 19:39
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

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment on lines 2372 to +2375
LOGGER.info(
"Removing tracking of replica: {} from storage utilization manager as this UNSUBSCRIBE is helix triggered action",
topicPartition);
"Removing replica: {} from PCS map and storage utilization manager. Trigger: {}",
topicPartition,
isDaVinciClient() ? "DaVinci client unsubscribe" : "Helix-triggered unsubscribe");
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The log message has a logical inconsistency. When isDaVinciClient() returns true, the ternary operator will always select "DaVinci client unsubscribe", even if the action is also Helix-triggered. This could be misleading in cases where a DaVinci client uses Helix-triggered unsubscribe.

Consider restructuring the log message to accurately reflect both conditions:

String trigger = isDaVinciClient() 
    ? (consumerAction.isHelixTriggeredAction() ? "DaVinci client (Helix-triggered)" : "DaVinci client") 
    : "Helix-triggered unsubscribe";
LOGGER.info(
    "Removing replica: {} from PCS map and storage utilization manager. Trigger: {}",
    topicPartition,
    trigger);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants