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.

Sign up for GitHub

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

Jump to bottom

feature: make undo log table name configurable #1392

Merged
merged 4 commits into from Aug 9, 2019
Merged

feature: make undo log table name configurable #1392

merged 4 commits into from Aug 9, 2019

Conversation

ggndnn
Copy link
Contributor

@ggndnn ggndnn commented Aug 2, 2019

Ⅰ. Describe what this PR did

make undo log table name configurable

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Aug 2, 2019 •
edited

Codecov Report

Merging #1392 into develop will decrease coverage by <.01%.
The diff coverage is 28.57%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1392      +/-   ##
=============================================
- Coverage      45.43%   45.43%   -0.01%     
  Complexity      1662     1662              
=============================================
  Files            345      345              
  Lines          12692    12699       +7     
  Branches        1608     1608              
=============================================
+ Hits            5767     5770       +3     
- Misses          6279     6282       +3     
- Partials         646      647       +1
Impacted Files Coverage Δ Complexity Δ
...ava/io/seata/core/constants/ConfigurationKeys.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...seata/rm/datasource/undo/UndoLogManagerOracle.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...va/io/seata/rm/datasource/undo/UndoLogManager.java 20.4% <100%> (+2.51%) 9 <0> (ø) ⬇️
...server/store/file/FileTransactionStoreManager.java 46.34% <0%> (-1.05%) 19% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4efbd27...b57cd3a. Read the comment docs.

slievrly
slievrly reviewed Aug 2, 2019
View reviewed changes
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

Why is there a need for this feature?

rm-datasource/src/main/java/io/seata/rm/datasource/undo/UndoLogManagerOracle.java Outdated
@@ -73,7 +75,8 @@ public int getValue() {
}
private static final Logger LOGGER = LoggerFactory.getLogger(UndoLogManagerOracle.class);

private static String UNDO_LOG_TABLE_NAME = "undo_log";
private static String UNDO_LOG_TABLE_NAME = ConfigurationFactory.getInstance()
Copy link
Member

Choose a reason for hiding this comment

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

final

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Add final to all constants.

core/src/main/java/io/seata/core/constants/ConfigurationKeys.java
/**
* The constant TRANSACTION_UNDO_LOG_DEFAULT_TABLE.
*/
public static final String TRANSACTION_UNDO_LOG_DEFAULT_TABLE = "undo_log";
Copy link
Member

Choose a reason for hiding this comment

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

DEFAULT_TRANSACTION_UNDO_LOG_TABLE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just like STORE_DB_GLOBAL_DEFAULT_TABLE or STORE_DB_BRANCH_DEFAULT_TABLE.

@ggndnn
Copy link
Contributor Author

ggndnn commented Aug 2, 2019

DB table naming specifications vary.

@ggndnn ggndnn force-pushed the feature_undo_log_table_configurable branch 2 times, most recently from 3e07575 to dc2e169 Compare August 2, 2019 14:42
@slievrly
Copy link
Member

slievrly commented Aug 3, 2019

naming

But after the table created, there is no need for the business side to maintain it.

@slievrly
Copy link
Member

slievrly commented Aug 5, 2019

@ggndnn please resolve conflicts.

@ggndnn ggndnn force-pushed the feature_undo_log_table_configurable branch from dc2e169 to 5fdb42b Compare August 5, 2019 04:01
@ggndnn ggndnn force-pushed the feature_undo_log_table_configurable branch from 5fdb42b to e12857c Compare August 5, 2019 04:14
@ggndnn
Copy link
Contributor Author

ggndnn commented Aug 5, 2019

Just give business side a chance to use different undo log table name other than undo_log.

@ggndnn
Copy link
Contributor Author

ggndnn commented Aug 5, 2019

@slievrly Conflicts resolved.

slievrly
slievrly approved these changes Aug 5, 2019
View reviewed changes
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

LGTM

sharajava
sharajava approved these changes Aug 7, 2019
View reviewed changes
Copy link
Contributor

@sharajava sharajava left a comment

Choose a reason for hiding this comment

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

okey to me

@ujjboy ujjboy merged commit 7103e54 into apache:develop Aug 9, 2019
@fescar-robot fescar-robot mentioned this pull request Aug 16, 2019
Closed
@wangliang181230 wangliang181230 added this to the 0.8.1 milestone Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharajava sharajava sharajava approved these changes

@slievrly slievrly slievrly approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ggndnn @codecov-io @slievrly @sharajava @ujjboy @wangliang181230