Skip to content
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.

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

Row-level TTL PR 2: TtlResolver and RemoteTableSpec cleanup #371

Merged
merged 5 commits into from
Oct 25, 2024

Conversation

ableegoldman
Copy link
Contributor

@ableegoldman ableegoldman commented Oct 24, 2024

Introduces the TtlResolver used to hook everything up within Responsive. Also cleans up the RemoteTableSpec

PR 1: API #370

Future TODO (in order)

  1. Cassandra Fact tables (insert/get)
  2. TopologyTestDriver
  3. Mongo KV tables (insert/get/range/all)
  4. Cassandra KVTables (insert/get/range/all)

@ableegoldman ableegoldman requested a review from agavra October 24, 2024 22:01
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM! This one is a quick one :) love more red lines than green ones

return ttlProvider.needsValueToComputeTtl();
}

public Optional<TtlDuration> computeTtl(final Bytes keyBytes, final byte[] valueBytes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a quick comment on why we need both of these APIs?

@ableegoldman ableegoldman merged commit f238757 into main Oct 25, 2024
1 check passed
@ableegoldman ableegoldman deleted the TTL-2-TtlResolver branch October 25, 2024 00:50
ableegoldman added a commit that referenced this pull request Oct 25, 2024
Quick followup to PR #2 (#371) which switched the method of setting a ttl from the old Spec layers to the new TtlResolver. Carried over the ttl to the CassandraFactTable but forgot to apply it for the CassandraKeyValueTable, which is done in this PR
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