-
Notifications
You must be signed in to change notification settings - Fork 50
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
Stream Inspector: Close inspector when connector/processor is deleted #882
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
cb47a02
processors closing inspectors
hariso 5468bd8
close inspector once
hariso 231b0f4
comments
hariso 537e893
connector
hariso e10ec9f
linter
hariso a2315d5
tests
hariso 93430f3
move closing to service
hariso 382e752
revert changes
hariso fbedaf8
added a test
hariso 75ac391
ptr to once
hariso 56831e5
comments
hariso ae722bc
tests, fixed bug
hariso 9a85a99
comments
hariso dcb127f
Merge branch 'main' into haris/close-inspector-session-on-delete
hariso d97b71d
close after delete
hariso 5ba9ac6
Merge branch 'main' into haris/close-inspector-session-on-delete
hariso 3bd50a2
Merge branch 'main' into haris/close-inspector-session-on-delete
hariso File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If we are strict, we should be locking
i.lock
before we loop throughi.sessions
, but thens.close()
also tries to lock it and we have a deadlock 😕 For correctness' sake we might want to lock first, then copyi.sessions
into a separate map/slice, then unlock and then loop through the copied map/slice to close the sessions. It still doesn't guarantee that a new session won't be created while this is happening.I know, it's an edge case, but I'm trying to say that something feels off regarding this design 😕
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 wasn't too happy with these locks from the beginning, and even less now. The thing is that the lock handles concurrent access to the sessions' map, but what we actually need to do is to make sure that the inspector will refuse to create a new session ever again after it's been closed.
IMHO, the edge case is rare enough that working more on this design doesn't return much, especially after we move
inspector.Close()
after connector/processor deletion. At that point, starting a new session won't be possible at all (the API will complain that the processor/connector doesn't exist).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 see, if we delete the entity first and then close the inspector, there can't be any more requests to create new sessions. Still, it doesn't feel right to have a component that can behave unexpectedly if it's used the wrong way and we rely on using it correctly elsewhere 😕 Don't worry, I won't block the PR because I feel bad 😅 I think we can do better though.
I thought about the locking and especially the optimization TODO in
Send
. I gave it a go and spiked out a slightly different approach to managing sessions in the inspector using a channel (see 2796796). The main difference would be that we need an upper bound on how many sessions we can create (I think it would be smart to have that anyway). We still can't get away without a lock, it's needed because we want to close the session channel in a different goroutine than the one that's writing to that channel, without a lock we risk a panic. The nice thing about this approach is that we don't need to lock anything to check for open sessions (using a map we don't get around that), so if there are no open sessions the performance is 2x as fast (and that's IMO the case we need to optimize for).There are some TODOs in that spike that would need to be addressed if we wanted to go down that route. Let me know what you think and if we should pursue this in a separate PR.
(BTW: I'll keep this comment unresolved so we can discuss it, but you can merge the PR anyway)
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 definitely agree with that and my point was that it reduces the changes of something which is an edge case already.
Really nice! 👍 It solves some of the annoyances we currently have. I was initially thinking about something like a slice or a channel to store the sessions, but didn't go with it because I wanted the sessions to be removed and not stay around after they are not needed anymore.
When you say "to check for open sessions" which part do you refer to?
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'm talking about this part where we return early if there are no open sessions. If we did the same thing with a map we would need to acquire a lock first.