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

feat: deprecated rpc method by adding deprecated. prefix to the rpc name #2251

Merged
merged 5 commits into from
Sep 6, 2020
Merged

feat: deprecated rpc method by adding deprecated. prefix to the rpc name #2251

merged 5 commits into from
Sep 6, 2020

Conversation

quake
Copy link
Member

@quake quake commented Aug 27, 2020 •
edited
Loading

resolve #2247, rpc method can be deprecated by adding deprecated. prefix to the rpc name:

+    #[rpc(name = "deprecated.get_cells_by_lock_hash")]
-    #[rpc(name = "get_cells_by_lock_hash")]
    fn get_cells_by_lock_hash

@quake quake requested a review from a team August 27, 2020 10:29
@quake quake requested a review from a team as a code owner August 27, 2020 10:29
@quake quake changed the title feat: allows deprecated rpc method by adding deprecated. prefix to the rpc name feat: deprecated rpc method by adding deprecated. prefix to the rpc name Aug 27, 2020
@doitian
Copy link
Member

doitian commented Aug 31, 2020

According to #2247, we need a window to notify that an RPC has been deprecated. During the window, users must be able to continue using the methods without any changes.

And I think renaming the RPC method name is not a friend deprecation mechanism, it requires updating client code. I prefer the solution used by bitcoin, in which user must listed the enabled deprecated methods in the command line arguments.

doitian
doitian requested changes Aug 31, 2020
View reviewed changes
Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

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

We need a two stage deprecation mechanism, see #2247

@quake
Copy link
Member Author

quake commented Aug 31, 2020

According to #2247, we need a window to notify that an RPC has been deprecated. During the window, users must be able to continue using the methods without any changes.

And I think renaming the RPC method name is not a friend deprecation mechanism, it requires updating client code. I prefer the solution used by bitcoin, in which user must listed the enabled deprecated methods in the command line arguments.

this PR doesn't rename rpc method, please review again, thanks.

doitian
doitian previously approved these changes Aug 31, 2020
View reviewed changes
yangby-cryptape
yangby-cryptape previously approved these changes Sep 2, 2020
View reviewed changes
@quake
Copy link
Member Author

quake commented Sep 3, 2020

bors r=doitian,yangby-cryptape

bors bot added a commit that referenced this pull request Sep 3, 2020
@bors @quake
Merge #2251
285f752 
2251: feat: deprecated rpc method by adding `deprecated.` prefix to the rpc name r=doitian,yangby-cryptape a=quake

resolve #2247, rpc method can be deprecated by adding `deprecated.` prefix to the rpc name:
```diff
+    #[rpc(name = "deprecated.get_cells_by_lock_hash")]
-    #[rpc(name = "get_cells_by_lock_hash")]
    fn get_cells_by_lock_hash
```

Co-authored-by: quake <quake.wang@gmail.com>
@bors
Copy link
Contributor

bors bot commented Sep 3, 2020

Build failed:

@quake quake dismissed stale reviews from yangby-cryptape and doitian via 51b8f25 September 3, 2020 10:08
@zhangsoledad
Copy link
Member

bors r=doitian,zhangsoledad,yangby-cryptape

@bors
Copy link
Contributor

bors bot commented Sep 6, 2020

Build succeeded:

@bors bors bot merged commit a2378b5 into develop Sep 6, 2020
@bors bors bot deleted the quake/deprecated_rpc branch September 6, 2020 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhangsoledad zhangsoledad zhangsoledad approved these changes

@doitian doitian doitian approved these changes

@keroro520 keroro520 Awaiting requested review from keroro520

@yangby-cryptape yangby-cryptape Awaiting requested review from yangby-cryptape

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

Successfully merging this pull request may close these issues.

CKB RPC Method Deprecation Process
4 participants
@quake @doitian @zhangsoledad @yangby-cryptape