server api without linearization #376
Conversation
|
@fxposter @jyotimahapatra I think the path forward is to test small changes like this in xds-relay staging, and try to instrument code if something goes wrong again. |
|
|
|
@jyotimahapatra do you use type urls not from this list for xds-relay: https://github.com/envoyproxy/go-control-plane/blob/master/pkg/resource/v2/resource.go#L16-L21 ? |
|
Also - do you use v2 or v3? |
|
We use eds/cds/lds/rds . We only use v3 |
|
@fxposter can you fix create_version? once it passes CI, I guess we can try it in staging. |
fxposter
force-pushed
the
new-server-api-only
branch
from
cdd4453 to
f16bfa0
Compare
|
@kyessenov sorry for it taking so long. @jyotimahapatra can we try testing this one? |
|
@kyessenov I thought a bit about why previous attempt could lead to system being overloaded and I think that https://github.com/envoyproxy/go-control-plane/pull/357/files#diff-d8ad87612eeb084487962f9097b132371b1cb8402432bc8ea190ab973fdba1feR223 - here we should do a slightly modified |
|
@jyotimahapatra can you, please, take a look? |
|
@jyotimahapatra @kyessenov Guys, can we finally proceed, please? |
|
I'd ping @jyotimahapatra or @jessicayuen how to get it tested on lyft's infrastructure. The code was fine to me, but I imagine the real tests are in the staging. |
|
Jyoti is no longer at Lyft, I'll take a look at this today or tomorrow. |
|
huh. lucky me that @kyessenov has another person to ping :) |
|
@samrabelachew pulled this change into xds-relay / Lyft and has been graciously testing on our staging traffic. So far things look promising, but we'd like to bake this over the weekend. If things look good, happy to approve and merge this! |
|
@fxposter We tested this internally and didn't see any problems. Notice this PR is still marked WIP, is that still the case? |
|
@jessicayuen nope, I just forgot to remove WIP after generating v3 version/etc. |
fxposter
changed the title
|
@kyessenov Did you want to take a pass before I merge this? |
Signed-off-by: Pavel Forkert <fxposter@gmail.com>
|
@snowp wow, merging incremental interfaces was fast. Should we maybe align them with the new way of what I propose here? |
Signed-off-by: Pavel Forkert <fxposter@gmail.com>
fxposter
force-pushed
the
new-server-api-only
branch
from
f16bfa0 to
d7620e8
Compare
|
@kyessenov rebased on top of current main branch. But not it looks weird, cause new "stream" apis are written in an original style, where chan is passed inside, but sotw apis are in new style. |
|
@fxposter Sorry I wasn't aware that we wanted to apply this to the Delta API as well, I was just trying to fast track the Delta work since it'd been stalled for so long. I don't feel particularly strongly about whether to land Delta as-is and make these interfaces updates later or to try to update it to use this asap, let's see what @alecholmez thinks since he's doing the work. |
|
@fxposter can you merge in the changes and we'll get this in? |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
@jyotimahapatra @kyessenov Followup of #375
Do you envision this to have any problems for clients to migrate to? Let's put linearization aside for now.