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

Add transaction manager config #239

Merged
merged 2 commits into from
Jul 9, 2021
Merged

Conversation

brfrn169
Copy link
Collaborator

@brfrn169 brfrn169 commented Jul 8, 2021

To support transactions through the Scalar DB server, I added a new transaction manager property (scalar.db.transaction_manager). We can set consensus-commit (default) or jdbc or grpc to this property. When we set grpc, we can perform transactions through the Scalar DB server. And if we set jdbc, we also need to set jdbc to scalar.db.storage.

I also removed the scalar.db.jdbc.transaction_manager_type property in this PR because we can replace it with the new property. So this is an incompatible change.

Please take a look!

@brfrn169 brfrn169 added the enhancement New feature or request label Jul 8, 2021
@brfrn169 brfrn169 requested a review from feeblefakie July 8, 2021 14:17
@brfrn169 brfrn169 self-assigned this Jul 8, 2021
feeblefakie
feeblefakie previously approved these changes Jul 9, 2021
Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!
Left one minor suggestion which I won't argue for not changing.

@@ -132,6 +139,35 @@ protected void load() {
namespacePrefix = Optional.empty();
}

if (Strings.isNullOrEmpty(props.getProperty(TRANSACTION_MANAGER))) {
transactionManagerClass = ConsensusCommitManager.class;
} else {
Copy link
Contributor

@feeblefakie feeblefakie Jul 9, 2021

Choose a reason for hiding this comment

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

Very minor suggestion. I won't argue against it.

transactionManagerClass = ConsensusCommitManager.class;
if (!Strings.isNullOrEmpty(props.getProperty(TRANSACTION_MANAGER))) {

@brfrn169 brfrn169 requested a review from feeblefakie July 9, 2021 11:09
@feeblefakie feeblefakie merged commit 77e047c into master Jul 9, 2021
@feeblefakie feeblefakie deleted the add-transaction-manager-config branch July 9, 2021 12:02
@brfrn169 brfrn169 added backward-incompatible and removed enhancement New feature or request labels Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants