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

chore(go-client): Merge go-client from XiaoMi/pegasus-go-client #980

Merged
merged 112 commits into from
May 26, 2022

Conversation

acelyc111
Copy link
Member

#945

Merge go-client from XiaoMi/pegasus-go-client

neverchanje and others added 30 commits December 28, 2017 17:51
Summary:
fix a serious bug that concurrent OpenTable operations to the same table invoke more than one QueryConfig rpcs. In this commit we added tests for this bug.
Add more tests for self-update.

Test Plan: N/A

Reviewers: qinzuoyan, sunweijie, laiyingchun, cailiuyang

Reviewed By: qinzuoyan

Subscribers: #pegasus

Tags: #pegasus

Differential Revision: https://phabricator.d.xiaomi.net/D79185
Summary: fix the bug that reads 4 additional bytes for response.

Test Plan: N/A

Reviewers: laiyingchun

Reviewed By: laiyingchun

Subscribers: #pegasus

Differential Revision: https://phabricator.d.xiaomi.net/D79820
Summary:
"session-auto-reconnect" functions as redialing the remote node
when the session was failed due to some io error. Since the original
design didn't take this into account, it assumed dialing is single-threaded.
However now multiple goroutines are able to start dialing, we need to
change the design.

In this commit we added a single-consumer channel for dialing, to ensure
there's only one goroutine to dial().

Because the session may become invalid after the user calls Close(). We
need to distinguish if the session is shutdown by io error or by Close().

For this sake we added state ConnStateClosed.

We also added several fixes:

- add write timeout
- sleep 1 sec after a failed write

Test Plan: N/A

Reviewers: #pegasus, qinzuoyan

Reviewed By: #pegasus, qinzuoyan

Subscribers: #pegasus

Differential Revision: https://phabricator.d.xiaomi.net/D80321
Summary:
- use enumer to generate ErrType masharlling and unmarshalling code.
- fix the bug that session could still be able to send requests after connection broke down, since ErrConnectionNotReady is retryable.
- improve error handling when errno in response header is not OK.
- no longer update configuration periodically, this is a design mistake.

Reviewers: #pegasus, laiyingchun

Reviewed By: #pegasus, laiyingchun

Subscribers: #pegasus

Differential Revision: https://phabricator.d.xiaomi.net/D81034
Reviewers: #pegasus, laiyingchun

Reviewed By: #pegasus, laiyingchun

Subscribers: #pegasus

Differential Revision: https://phabricator.d.xiaomi.net/D81204
and:
- print error message for illed self-update
- remove stream_in_v2
and
- add comment on OpenTable, recommend the user to reuse the returned table instance.
Some user of go-client encountered with version conflict with older version of thrift.
Generally nested vendor is not an prefered way to resolve version conflict,
but since the idl/ contains thrift generated code, the corresponding
version of thrift library is also required.
and:
- alarm if the given meta server list is empty
- remove onebox
Previously in stream_in.go and stream_out.go we call os read and write
once per rpc, without consideration for partial read/write of TCP.
In this commit we loop for every partial read/write until they are fully
finished.
[go-client] fix serious bug in low level RPC implementation.
and remove glide.yaml: no need to maintain dependencies by lib.
remove replica connections when meta server removes them in configuration
…ector.Close.

1. Closing table shouldn't close the connections, which may be still in used by other tables.
   We add an unit test for this(TestPegasusTableConnector_Close).
2. Connections are also unremovable when routing table doesn't contain it, because they may
   still occupied by other table. Therefore we revert pr#3.
bugfix: revert pr#3 and fix connections being closed during TableConn…
Currently we never delay for any configuration update, when it's indeed necessary.
Update configuration only when it's indeed necessary.
- initialize logger when creating PegasusCodec to avoid read lock in logging.
- read message begin after error code is read, even when it's not ok
- table_connector: don't add gpid & replica to error when err is nil
- session: marshall rpc by caller to improve throughput
@acelyc111 acelyc111 changed the title chore: Merge go-client from XiaoMi/pegasus-go-client chore(go-client): Merge go-client from XiaoMi/pegasus-go-client May 25, 2022
@acelyc111
Copy link
Member Author

acelyc111 commented May 25, 2022

My check list:

  • no functional changes
  • new actions for go-client added
  • all actions passed
  • APLv2 license header added to all files, or reasonable configs added to .licenserc.yaml
  • Copyright of xiaomi has been removed
  • github.com/pegasus-kv/thrift updated to github.com/apache/thrift
  • xiaomi updated to apache
    • go package urls: not changed. If change to apache, make will fail, we have to do this change in the next PR
    • PEGASUS_PKG_URL: not changed. I added TODO to do it next time.
    • urls in README.md: not changed. Since go-client is not released on apache repo yet.
    • go-client/docs/how_to_add_new_interface.md: not changed. Same.
  • package is not released to pkg.go.dev

@acelyc111 acelyc111 force-pushed the merge-go-client-new branch 2 times, most recently from 9302d46 to 876eb2d Compare May 25, 2022 15:56
foreverneverer
foreverneverer previously approved these changes May 26, 2022
levy5307
levy5307 previously approved these changes May 26, 2022
@acelyc111 acelyc111 dismissed stale reviews from levy5307 and foreverneverer via 86008c5 May 26, 2022 02:42
@acelyc111 acelyc111 force-pushed the merge-go-client-new branch from 876eb2d to 86008c5 Compare May 26, 2022 02:42
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.

9 participants