-
Notifications
You must be signed in to change notification settings - Fork 58
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
Client: offer all supported serializers #270
Conversation
ping @gammazero, can you please take a look at this one ? Also I'd like to help with the maintenance of Nexus. I have a bunch of features planned that I'd like to implement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, and I intend to merge it after a few more tests.
As far as maintaining this code, I am considering a fairly significant overhaul. Many of the APIs and configurations can be simplified, as well as changes to the internal queuing.
@@ -691,7 +691,7 @@ func (ks *serverKeyStore) AuthRole(authid string) (string, error) { | |||
|
|||
func TestConnectContext(t *testing.T) { | |||
const ( | |||
expect = "dial tcp: lookup localhost: operation was canceled" | |||
expect = "dial tcp: operation was canceled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the better way to fix this is to either change CI to use a later version of go, or have the test look for "operation was canceled"
Right. The authentication API in both the client and server API can be simplified. I have already made some progress on that front, will share the changes I made. |
Regarding internal queuing. I believe that are some bottlenecks around concurrency, in my benchmarks, the RPC/second count doesn't gain significantly as the number of goroutines increases. On a single core I could do 40k+ RPC/second (empty payload) but as the concurrency is increased, we can clearly see diminishing returns |
Without changing the user API, takes care of #260
If no serializer is provided in the config: offer all supported
If specific serializer is request, keep the existing behavior