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

server: sotw dedupe and linearization #451

Merged
merged 7 commits into from
Jan 6, 2022
Merged

server: sotw dedupe and linearization #451

merged 7 commits into from
Jan 6, 2022

Conversation

alecholmez
Copy link
Contributor

Signed-off-by: Alec Holmes [email protected]

I closed my previous sotw dedupe PR in favor of this one since we merged in the linearization API code which this now adheres too. Still need to benchmark

@alecholmez alecholmez mentioned this pull request Jun 29, 2021
Open
@dontlaugh
Copy link

@alecholmez you want to do benchmarks for this, right

@alecholmez
Copy link
Contributor Author

@dontlaugh yup, we'd like to see how this compares with the extra go routine overhead to the current system

@github-actions
Copy link

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!

@github-actions github-actions bot added the stale label Jul 31, 2021
@alecholmez alecholmez changed the title sotw dedupe and linearization server: sotw dedupe and linearization Aug 9, 2021
@alecholmez
Copy link
Contributor Author

@snowp a review here would also be great. I revisited this a went to your reflect method you suggested in the issue you opened. I'm going to run some pprof profiling on this to see what the new code looks like as its running

@alecholmez alecholmez requested a review from snowp October 18, 2021 15:38
snowp
snowp reviewed Oct 19, 2021
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks this looks good for the most part, just a few comments

pkg/server/sotw/v3/watches.go Outdated Show resolved Hide resolved
pkg/server/sotw/v3/watches.go Outdated Show resolved Hide resolved
pkg/server/sotw/v3/watches.go Outdated Show resolved Hide resolved
pkg/server/sotw/v3/watches.go Outdated Show resolved Hide resolved
pkg/server/sotw/v3/watches.go Outdated Show resolved Hide resolved
pkg/server/sotw/v3/watches.go Outdated Show resolved Hide resolved
pkg/server/v3/server_test.go Outdated Show resolved Hide resolved
pkg/server/v3/server_test.go Outdated Show resolved Hide resolved
pkg/server/v3/server_test.go Outdated Show resolved Hide resolved
pkg/server/v3/server_test.go Outdated Show resolved Hide resolved
snowp
snowp previously requested changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Seems like CI is red?

@alecholmez
Copy link
Contributor Author

@snowp Just addressed the last of the requests, I removed the instances of the 2 default channels but kept them in the Recompute. Also pulled out seleteCase so I think it's all good now. Let me know if theres anything else before we merge

jpeach
jpeach requested changes Oct 27, 2021
View reviewed changes
pkg/server/sotw/v3/watches.go Outdated Show resolved Hide resolved
pkg/server/sotw/v3/server.go Outdated Show resolved Hide resolved
pkg/server/sotw/v3/server.go Outdated Show resolved Hide resolved
pkg/server/sotw/v3/watches.go Outdated Show resolved Hide resolved
pkg/server/sotw/v3/server.go Outdated Show resolved Hide resolved
@alecholmez
Copy link
Contributor Author

alecholmez commented Oct 28, 2021 •
edited
Loading

Just FYI i'll get back to on this shortly (probably this weekend), I want to cut the v0.10.0 today so we can land this after since I imagine this change will need to be made in Delta as well

@alecholmez
dedupe and linearization
0817656 
Signed-off-by: Alec Holmes <[email protected]>

rebase and fix errors

Signed-off-by: Alec Holmes <[email protected]>
@alecholmez
move to reflect select and reduce go routine spawning
9a7589b 
Signed-off-by: Alec Holmes <[email protected]>
@alecholmez
review comments
8e3e122 
Signed-off-by: alecholmez <[email protected]>

move back recompute

Signed-off-by: alecholmez <[email protected]>
@alecholmez
remove selectCase and remove any signs of 2 defaults
eba7aed 
Signed-off-by: alecholmez <[email protected]>

oops remove that

Signed-off-by: alecholmez <[email protected]>

stash changes

Signed-off-by: alecholmez <[email protected]>
@alecholmez alecholmez requested a review from snowp December 6, 2021 18:23
@alecholmez
Copy link
Contributor Author

@jpeach can you give this another pass when you get some free time?

jpeach
jpeach previously approved these changes Dec 15, 2021
View reviewed changes
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

👍

@alecholmez
Copy link
Contributor Author

@snowp want to give the final stamp on this one?

@jpeach jpeach mentioned this pull request Dec 20, 2021
Closed
@rueian rueian mentioned this pull request Dec 21, 2021
Open
@rueian
Copy link

rueian commented Dec 21, 2021

Hi @alecholmez, @jpeach,

I based on this PR to solve another deadlock in sotw server (#530, #531). Please take a look.

@alecholmez
Copy link
Contributor Author

@jpeach is this good to merge? @rueian once this is merged can you rebase your branch off main instead?

@jpeach
Copy link
Contributor

jpeach commented Jan 6, 2022

@jpeach is this good to merge?

Yup!

@alecholmez alecholmez merged commit 01bb8ac into main Jan 6, 2022
@alecholmez alecholmez deleted the sotw-dedupe branch January 6, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpeach jpeach jpeach approved these changes

@snowp snowp Awaiting requested review from snowp

Assignees
No one assigned
Labels
area/sotw design proposal no stalebot
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alecholmez @dontlaugh @rueian @jpeach @snowp