chore: new proto changes; read session heartbeat

[sgbalogh]

Closes #13

---

Important

Introduces heartbeat support in ReadSession, updates BasinInfo to use BasinScope enum, and adjusts logging and version.

• ReadSession Heartbeat:
  • Added heartbeat support in ReadSession with livenessDaemon() and scheduleLivenessCheck() methods.
  • ReadSessionRequest updated to include heartbeats boolean.
  • Heartbeat logging added in readSessionInner().
• BasinInfo and BasinScope:
  • BasinInfo now uses BasinScope enum instead of String for scope.
  • New BasinScope enum created with values UNSPECIFIED, AWS_US_EAST_1, and UNKNOWN.
• Logging and Version:
  • Changed logger level to trace for s2 in logback.xml.
  • Updated version to 0.0.14 in gradle.properties.

^{This description was created by for be93697. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to d61eac6 in 2 minutes and 37 seconds

More details

• Looked at 288 lines of code in 7 files
• Skipped 0 files when reviewing.
• Skipped posting 11 drafted comments based on config settings.

1. app/src/main/java/org/example/app/ManagedReadSessionDemo.java:52

• Draft comment:
  Using withHeartbeats(true) replaces the read limit configuration. Confirm that removing the read limit is intentional and that the default (ReadLimit.NONE) in the builder is acceptable.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment is asking for confirmation/verification, which violates our rules. It's speculative ("is acceptable") rather than pointing out a definite issue. While it provides information about the implications of the change, it doesn't identify a clear problem that needs fixing.
  The comment does point out a potentially important change in behavior - removing read limits could affect system resource usage. Maybe this is a serious enough change to warrant discussion?
  While the behavior change is notable, the rules explicitly state not to ask for confirmations or verifications. If removing the read limit was problematic, the comment should directly state the issue and propose a fix.
  Delete the comment as it violates rules against asking for confirmations and making speculative comments. If there's a concrete issue with removing read limits, it should be stated directly.

2. app/src/main/resources/logback.xml:10

• Draft comment:
  Logger level for 's2' was changed to trace. Ensure this verbosity is desired in the deployed environment.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

3. s2-sdk/src/main/java/s2/client/ReadSession.java:25

• Draft comment:
  Consider using the primitive 'long' instead of 'Long' for HEARTBEAT_THRESHOLD_NANOS to avoid unnecessary boxing.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

4. s2-sdk/src/main/java/s2/client/ReadSession.java:105

• Draft comment:
  In scheduleLivenessCheck, the lambda adds a listener to cancel the scheduled check once livenessFuture completes; ensure this cancellation is sufficient and does not cause race conditions.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

5. s2-sdk/src/main/java/s2/client/ReadSession.java:115

• Draft comment:
  On heartbeat timeout (delay <= 0), the code calls this.daemon.cancel(true). Verify that cancelling the retry daemon here is the correct error handling behavior.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify if cancelling the retry daemon is the correct behavior. This falls under the rule of not asking the author to confirm their intention or ensure the behavior is intended. Therefore, this comment should be removed.

6. s2-sdk/src/main/java/s2/types/ReadSessionRequest.java:50

• Draft comment:
  The builder now includes an option for heartbeats. Confirm that the default heartbeats value (false) is appropriate where not specified.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

7. app/src/main/java/org/example/app/ManagedReadSessionDemo.java:52

• Draft comment:
  Changed the session request builder to use heartbeats (withHeartbeats(true)) instead of a read limit. Confirm that the backend service correctly interprets heartbeats for timeout logic.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

8. app/src/main/resources/logback.xml:10

• Draft comment:
  Logger 's2' is now set to trace level. Ensure this verbosity is acceptable for production, or adjust accordingly.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

9. s2-sdk/src/main/java/s2/client/ReadSession.java:25

• Draft comment:
  Consider using the primitive type 'long' instead of the boxed Long for HEARTBEAT_THRESHOLD_NANOS to avoid unnecessary boxing.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

10. s2-sdk/src/main/java/s2/client/ReadSession.java:104

• Draft comment:
  The recursive scheduling in scheduleLivenessCheck effectively rechecks heartbeat liveness. Verify that under long-lived sessions this approach does not overload the executor.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

11. s2-sdk/src/main/java/s2/types/ReadSessionRequest.java:50

• Draft comment:
  The heartbeat flag is now added to the builder and properly propagated via update() and toProto(). This implementation meets the requirement.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

Workflow ID: wflow_KRgD1270DVF60i1b

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on be93697 in 53 seconds

More details

• Looked at 9 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. gradle.properties:1

• Draft comment:
  Version update: Removed '-SNAPSHOT'. Confirm that this change aligns with the intended release process. (No heartbeat code changes observed.)
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. gradle.properties:1

• Draft comment:
  Version updated from 0.0.14-SNAPSHOT to 0.0.14. However, the PR title and description mention implementing heartbeat logic for ReadSession, but no such changes appear in this diff.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  This comment is asking about PR description completeness and making assumptions about what should be in the PR. The rules explicitly state not to make assumptions about PR descriptions or ask for updates to them. Version changes are also dependency-related, which we're told to ignore.
  Maybe the version change is actually incorrect and needs to be reverted? Maybe this indicates a larger process issue?
  Per the rules, we should not comment on dependency/version changes or PR descriptions, regardless of potential process issues.
  This comment should be deleted as it violates two rules: no commenting on PR descriptions and no commenting on version/dependency changes.

Workflow ID: wflow_6hHsGgUlMsZ0fHq4

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.