Skip to content

server: sotw dedupe and linearization#451

Merged
alecholmez merged 7 commits intomainfrom
sotw-dedupe
Jan 6, 2022
Merged

server: sotw dedupe and linearization#451
alecholmez merged 7 commits intomainfrom
sotw-dedupe

Conversation

@alecholmez
Copy link
Contributor

@alecholmez alecholmez commented Jun 21, 2021

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

dontlaugh commented Jul 1, 2021

@alecholmez you want to do benchmarks for this, right

@alecholmez
Copy link
Contributor Author

alecholmez commented Jul 1, 2021

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

@github-actions
Copy link

github-actions bot commented Jul 31, 2021

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

alecholmez commented Oct 15, 2021

@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

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

alecholmez commented Oct 26, 2021

@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
@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

alecholmez commented Dec 14, 2021

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

jpeach
jpeach reviewed Dec 15, 2021
View reviewed changes
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

alecholmez commented Dec 15, 2021

@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

alecholmez commented Jan 5, 2022

@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