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

Pr fix connect server #703

Merged

Conversation

Bolodya1997
Copy link

@Bolodya1997 Bolodya1997 commented Feb 10, 2021

Continues #700.
Closes #704.

@Bolodya1997 Bolodya1997 marked this pull request as draft February 10, 2021 16:15
@Bolodya1997 Bolodya1997 marked this pull request as ready for review February 11, 2021 11:27
@Bolodya1997 Bolodya1997 force-pushed the pr-fix-connect-server branch 3 times, most recently from 667e632 to a6affcc Compare February 11, 2021 12:45
@Bolodya1997 Bolodya1997 marked this pull request as draft February 11, 2021 14:25
@Bolodya1997 Bolodya1997 force-pushed the pr-fix-connect-server branch 2 times, most recently from b099c6c to 9e22e71 Compare February 12, 2021 03:53
@Bolodya1997 Bolodya1997 marked this pull request as ready for review February 12, 2021 03:55
@denis-tingaikin
Copy link
Member

@edwarnicke Could you take a look at this patch that's solve issue #704?

Any thoughts are welcome :)

@denis-tingaikin
Copy link
Member

denis-tingaikin commented Feb 16, 2021

@edwarnicke , @Bolodya1997 I think this also can be simplified if we will use a refcountmap that allows deleting entry without looking at ref count. What do you think?

@artem-belov artem-belov mentioned this pull request Feb 25, 2021
@@ -75,11 +75,16 @@ func (u *clientURLClient) init() error {
u.dialErr = errors.New("cannot dial nil clienturl.ClientURL(ctx)")
return
}

ctx, cancel := context.WithTimeout(u.ctx, dialTimeout)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we introducing a new arbitrary dialTimeout instead of deriving it from the ctx of the request?

Copy link
Author

Choose a reason for hiding this comment

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

It will break discover + roundrobin, because it will wait for the first dial until requests lives and so there won't be any chance to try someone else.

)

type serializeClient struct {
executor multiExecutor
executor multiexecutor.MultiExecutor
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a separate PR to introduce multiexecutor into serialize. Could we split it out?

@edwarnicke
Copy link
Member

One way or the other, we need to find a way to not be queueing up all Requests/Closes globally in connect. Its fine to queue them up by Connection.ID... but not globally. I think we can get there with a combination of sync.Map and atomics.

Copy link
Member

@edwarnicke edwarnicke left a comment

Choose a reason for hiding this comment

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

Happy to merge once its rebased to avoid merge conflicts.

Artem Belov added 2 commits March 4, 2021 12:54
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Artem Belov and others added 14 commits March 4, 2021 12:56
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Artem Belov <artem.belov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 marked this pull request as draft March 4, 2021 06:56
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 marked this pull request as ready for review March 4, 2021 09:55
@edwarnicke edwarnicke merged commit 68e5877 into networkservicemesh:master Mar 4, 2021
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.

Connect server doesn't handle GRPC connection error
4 participants