iting0321 commented on PR #5135: URL: https://github.com/apache/polaris/pull/5135#issuecomment-5082379289
> I don’t think this is ready to merge yet. > > NoSQL pagination tokens contain a reference to historic state, subsequent requests need that exact object. If maintenance has already collected it, pagination would break in a subtle way, after an arbitrarily short time. > > Could we define the expected token behavior across maintenance and add a test covering this interaction? Depending on the outcome, we may need to preserve a time-based retention dimension, though not necessarily CEL itself. > > Please also verify the migration mapping for `commits <= N`: since the current commit is retained before evaluating the predicate, the old expression appears to retain `N + 1`, not `N`. > > Requesting changes pending that investigation. Hi @snazy, thanks for your feedback! I’ve updated the PR accordingly. For pagination, I defined the following behavior: * A NoSQL pagination token remains usable for at least the configured `pagination-token-retention` duration after its referenced snapshot is superseded. * The new setting applies globally to all maintained container histories, rather than only catalog state, and defaults to `P30D`. * Commit-count and time-based retention are combined with OR semantics: a snapshot is retained while either condition requires it. * After the retention window expires and maintenance collects the snapshot, reuse of the token fails explicitly with `400 Bad Request` (`Invalid or expired NoSQL pagination token`) instead of silently returning an empty page. I added an integration test that creates a pagination token, supersedes its snapshot, runs maintenance, and verifies that: 1. pagination remains snapshot-consistent within the retention window 2. newly created data does not appear in the historic page 3. the token is rejected explicitly after the window expires and the snapshot is collected. You were also correct about the migration mapping. Because the current commit is retained before the predicate is evaluated: * `commits < N` maps to `N` * `commits <= N` maps to `N + 1` I corrected this in both the changelog and migration documentation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
