Use include_metadata configuration in Consensus Commit

[brfrn169]

Description

After the change in scalar-labs/scalardb#2798, when an uncommitted record is read, the UncommittedRecordException is no longer thrown immediately if the corresponding coordinator record exists, and then, lazy recovery is performed asynchronously.

The current read-all logic in the transfer test uses the Storage API and relies on the previous behavior where the exception was thrown immediately upon reading an uncommitted record. This approach is incompatible with the new read logic.

To address this issue, this PR updates the read-all logic to use the include_metadata configuration (scalar.db.consensus_commit.include_metadata.enabled) in Consensus Commit to include transaction metadata in the records, instead of relying on the Storage API.

Related issues and/or PRs

• Improve read process in Consensus Commit scalardb#2798

Changes made

• Updated the read-all logic to use the include_metadata configuration (scalar.db.consensus_commit.include_metadata.enabled) in Consensus Commit.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

• I have commented my code, particularly in hard-to-understand areas.
• I have updated the documentation to reflect the changes.
• Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
• Tests (unit, integration, etc.) have been added for the changes.
• My changes generate no new warnings.
• Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

[Copilot]

Pull Request Overview

This PR updates the read-all logic in the Consensus Commit module to use the include_metadata configuration instead of relying on the Storage API. Key changes include removing storage dependency checks from tests and core logic, updating configuration properties in db_extend and cluster files, and adapting the get-all calls in transfer and transfer-2pc tests.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
scalardb/test/scalardb/transfer_test.clj	Removed references to Storage API checks and mock-storage usage.
scalardb/test/scalardb/transfer_2pc_test.clj	Removed Storage API dependency from transaction tests.
scalardb/src/scalardb/transfer.clj	Updated read-all logic by removing Storage API calls to use transaction API exclusively.
scalardb/src/scalardb/db_extend.clj	Added include_metadata configuration to the coordinator properties.
scalardb/src/scalardb/db/cluster.clj	Updated cluster configuration to include the new include_metadata setting.

Comments suppressed due to low confidence (4)

scalardb/src/scalardb/transfer.clj:148

• [nitpick] Since the Storage API is no longer used in this context, update the inline comments and documentation to reflect the new include_metadata configuration usage.

  (scalar/check-transaction-connection! test)

scalardb/test/scalardb/transfer_test.clj:176

• [nitpick] Update this test assertion to match the new behavior where the Storage API connection check is no longer invoked for get-all calls.

                                 nil nil)

scalardb/src/scalardb/db_extend.clj:33

• [nitpick] Ensure that the new include_metadata property is clearly documented in the configuration and user documentation to aid future maintainers.

    (.setProperty "scalar.db.consensus_commit.include_metadata.enabled" "true")))

scalardb/src/scalardb/db/cluster.clj:43

• The configuration array contains an empty string entry which appears to be unintentional. Removing the empty string would help prevent potential misconfigurations.

               ""

[gemini-code-assist]

Summary of Changes

Hello @brfrn169, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves an incompatibility issue in the read-all logic of the transfer test suite, which arose from recent changes in how uncommitted records are handled. By enabling the include_metadata configuration in Consensus Commit, the system can now retrieve necessary transaction metadata directly through the transaction API, thereby removing the need for the read-all function to rely on the separate Storage API. This change simplifies the codebase by streamlining data access and updating test configurations to reflect the new, more integrated approach.

Highlights

• Configuration Update: Enabled the scalar.db.consensus_commit.include_metadata.enabled property in both the cluster configuration (scalardb/src/scalardb/db/cluster.clj) and the programmatic setup (scalardb/src/scalardb/db_extend.clj) to ensure transaction metadata is included in records.
• read-all Logic Refinement: Modified the read-all function in scalardb/src/scalardb/transfer.clj to directly use the transaction API for fetching records, eliminating the previous dependency on the separate Storage API for reading transaction metadata.
• Dependency Removal: Removed all explicit dependencies on the Storage API within the transfer module's read-all-with-retry function and corresponding unit tests (scalardb/test/scalardb/transfer_2pc_test.clj, scalardb/test/scalardb/transfer_test.clj), including imports, mock objects, and related assertions.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request effectively updates the read-all logic in Consensus Commit to leverage the include_metadata configuration, aligning with the new behavior where UncommittedRecordException is no longer immediately thrown. The changes correctly remove the dependency on the Storage API for reading transaction metadata, simplifying the read-all function and its associated test setups. The configuration updates and test cleanups are consistent with the described intent and appear to be well-implemented.

[brfrn169]

Set the scalar.db.consensus_commit.include_metadata.enabled property to true to include transaction metadata in the records.

[brfrn169]

I think this change also affects the cluster test, so I added the configuration here as well.

Note that I didn't add support for the read-committed isolation level and the one-phase commit optimization in the cluster test.

[brfrn169]

Removed the use of the Storage API.

[brfrn169]

@yito88 After this change, I believe we can remove the Storage API parts from the ScalarDB Jepsen tests. Is my understanding correct?

[yito88]

Looks good!
include_metadata is what I wanted! Thanks!

I'll clean the Storage API related parts up.

P.S. We still need the Storage API to access the coordinator table to check the transaction status.

scalar-jepsen/scalardb/src/scalardb/core.clj

Line 228 in 5d40176

(let [coordinator (Coordinator. @(:storage test))

[feeblefakie]

LGTM! Thank you!