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

updated connector structure #30075

Closed
wants to merge 1 commit into from
Closed

Conversation

akats7
Copy link
Contributor

@akats7 akats7 commented Dec 19, 2023

@djaglowski wanted to get your thoughts on this. I did some refactoring to improve the separation of concerns as we discussed in PR2. In this structure all retry logic and index manipulation logic has been moved to the pipelineSelector as to your point all of the retry logic is tightly coupled with the indexes. Now there are only two exposed methods in pipelineSelector, the main one just returning the current selected pipeline.

The issue I was having is that in order to avoid race conditions, we needed a way to get " a snapshot" of the indexes/state at the time that an error was triggered without inconsistent state or blocking. Which is why in the original design the index was returned from getCurrentConsumer, and then passed back into the failover component in reportStable and handlePipelineError.

In the update I made getCurrentConsumer will now return a channel along with the consumer, and a bool value written to the channel will serve as the signal of whether or not an error was returned. And all of the retry logic will be contained in pipelineSelector based on the reads from the channels.

The channels are mapped one to one to consumers/pipelines so they are dynamic in quantity. I implemented this with reflection which seemed better than alternatives like having a goroutine per channel.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Really appreciate you working this out. Seems much easier to reason about at the connector level. Generally looks good to me but I see unit tests are failing on a race condition so not sure it's working as intended.

@akats7
Copy link
Contributor Author

akats7 commented Jan 8, 2024

Really appreciate you working this out. Seems much easier to reason about at the connector level. Generally looks good to me but I see unit tests are failing on a race condition so not sure it's working as intended.

Hey @djaglowski, sure thing, yep just wanted to get your thoughts on the idea before addressing it further. Will push up an update.

On a side note, am I able to apply for Org membership and be made a code owner for the component?

@djaglowski
Copy link
Member

On a side note, am I able to apply for Org membership and be made a code owner for the component?

I would think so. I'm happy to be one sponsor. You'll need a second, which can be any approver/maintainer on any repo in the org.

@akats7
Copy link
Contributor Author

akats7 commented Jan 8, 2024

On a side note, am I able to apply for Org membership and be made a code owner for the component?

I would think so. I'm happy to be one sponsor. You'll need a second, which can be any approver/maintainer on any repo in the org.

Gotcha, sounds good. @breedx-splk would you be able to sponsor me based on our collaboration on the jmx gatherer.

@breedx-splk
Copy link
Contributor

On a side note, am I able to apply for Org membership and be made a code owner for the component?

I would think so. I'm happy to be one sponsor. You'll need a second, which can be any approver/maintainer on any repo in the org.

Gotcha, sounds good. @breedx-splk would you be able to sponsor me based on our collaboration on the jmx gatherer.

Yes, I would sponsor @akats7 as codeowner for the failover connector. Thanks!

@djaglowski
Copy link
Member

@akats7, you'll have to open an issue on the community repo according to the instructions here: https://github.com/open-telemetry/community/blob/main/community-membership.md#requirements

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jan 26, 2024
@breedx-splk
Copy link
Contributor

@akats7 is a member now: open-telemetry/community#1872
Where does that leave this?

@akats7
Copy link
Contributor Author

akats7 commented Feb 7, 2024

@akats7 is a member now: open-telemetry/community#1872 Where does that leave this?

Hey @breedx-splk, this PR can be closed, this functionality was added in a different PR that’s been merged already

@akats7 akats7 closed this Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants