Skip to content

[client][da-vinci-client] Add seek to tails API in Seeking DVC client #2296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

[client][da-vinci-client] Add seek to tails API in Seeking DVC client #2296

wants to merge 17 commits into from

Conversation

@majisourav99
Copy link
Contributor

@majisourav99 majisourav99 commented Nov 18, 2025 •
edited
Loading

Problem Statement

Add seek to tails API in Seeking DVC DRVR client to existing set of other seep APIs like seek by check point and seek by timestamps.

Solution

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.

@majisourav99 majisourav99 changed the title [client [client][da-vinci-client] Add seek to tails API in Seeking DVC client Nov 18, 2025
@majisourav99 majisourav99 force-pushed the seeking branch 2 times, most recently from c7f2820 to 91a37de Compare November 21, 2025 00:29
kvargha
kvargha reviewed Nov 21, 2025
View reviewed changes
clients/da-vinci-client/src/main/java/com/linkedin/davinci/StoreBackend.java
if (targetRegions.contains(currentRegion) || startIngestionInNonTargetRegion || !isTargetRegionEnabled) {
LOGGER.info("Subscribing to future version {}", targetVersion.kafkaTopicName());
setDaVinciFutureVersion(new VersionBackend(backend, targetVersion, stats));
// For future version subscription, we don't need to pass any timestamps or position map
Copy link
Contributor

@kvargha kvargha Nov 21, 2025

Choose a reason for hiding this comment

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

Nit: Can we update this comment please

...ts/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java
Comment on lines +2307 to +2312
// report completion immediately for user seek subscription
partitionConsumptionStateMap.get(partition).lagHasCaughtUp();
reportCompleted(partitionConsumptionStateMap.get(partition), true);
Copy link
Contributor

@kvargha kvargha Nov 21, 2025

Choose a reason for hiding this comment

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

Won't this only be true for seekToTail? What if a user passes in 0 into seekToTimestamp?

...rc/integrationTest/java/com/linkedin/venice/endToEnd/DaVinciClientRecordTransformerTest.java Outdated
Comment on lines 918 to 922
for (DefaultPubSubMessage message: messages) {
System.out.println(
"position: " + message.getPosition() + "ts11 "
+ message.getValue().getProducerMetadata().getMessageTimestamp());
}
Copy link
Contributor

@kvargha kvargha Nov 21, 2025

Choose a reason for hiding this comment

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

Cleanup?

@majisourav99 majisourav99 force-pushed the seeking branch 2 times, most recently from db91680 to d1200b4 Compare November 25, 2025 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kvargha kvargha kvargha left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@majisourav99 @kvargha