Skip to content

Conversation

mxsm
Copy link
Contributor

@mxsm mxsm commented Jul 4, 2022

Fix(173) DLedgerServer The code is not formatted correctly and Correct incorrect words in comment lines

Copy link
Contributor

@tsunghanjacktsai tsunghanjacktsai left a comment

Choose a reason for hiding this comment

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

Pretty detailed changes. Appreciate it.

Copy link
Contributor

@RongtongJin RongtongJin left a comment

Choose a reason for hiding this comment

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

Except for compatibility problems, other code is good

Comment on lines 444 to 473
public DLedgerStore getdLedgerStore() {
public DLedgerStore getDLedgerStore() {
return dLedgerStore;
}

public DLedgerRpcService getdLedgerRpcService() {
public DLedgerRpcService getDLedgerRpcService() {
return dLedgerRpcService;
}

public DLedgerLeaderElector getdLedgerLeaderElector() {
public DLedgerLeaderElector getDLedgerLeaderElector() {
return dLedgerLeaderElector;
}

public DLedgerConfig getdLedgerConfig() {
public DLedgerConfig getDLedgerConfig() {
return dLedgerConfig;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Directly modifying these interface names will cause compatibility problems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add @deprecated annotation for those methods instead of Directly modifying these interface names. and add a new method like getDLedgerConfig to solution compatibility problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Directly modifying these interface names will cause compatibility problems

I search found that RocketMQ uses these methods, and it does cause compatibility issues if you change them directly.add @deprecated annotation for those methods instead of Directly modifying these interface names. and add a new method like getDLedgerConfig to solution compatibility problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Directly modifying these interface names will cause compatibility problems

I search found that RocketMQ uses these methods, and it does cause compatibility issues if you change them directly.add @deprecated annotation for those methods instead of Directly modifying these interface names. and add a new method like getDLedgerConfig to solution compatibility problems.

Good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Directly modifying these interface names will cause compatibility problems

I search found that RocketMQ uses these methods, and it does cause compatibility issues if you change them directly.add @deprecated annotation for those methods instead of Directly modifying these interface names. and add a new method like getDLedgerConfig to solution compatibility problems.

Good idea!

I solved the compatibility problem and The code was resubmitted

@RongtongJin RongtongJin merged commit 167f4d8 into openmessaging:master Jul 13, 2022
@mxsm mxsm deleted the dledger-173 branch July 13, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants