Skip to content

fix(ingestor): drop the hardcoded fetch failure channel size - #70

Closed
arn-alt wants to merge 1 commit into
criteo:mainfrom
arn-alt:fix/fetch-failure-channel-size
Closed

arn-alt wants to merge 1 commit into
criteo:mainfrom
arn-alt:fix/fetch-failure-channel-size

Conversation

@arn-alt

@arn-alt arn-alt commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

AI assisted.

The buffer size was 7 for 9 fetches. The channel is drained while the fetches run, so it needs no buffer, and no count to keep in sync. A full DA1 build against a local Netbox succeeds.

AI assisted.

ingestorNumber was 7 for 9 fetches. The channel is drained while the
fetches run, so it needs no buffer, and no count to keep in sync.
"github.com/criteo/data-aggregation-api/internal/report"
)

const ingestorNumber = 7

@kpetremann kpetremann Oct 8, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Even if this number should be updated (it will in the interfaces PR), I don't see how it is causing an issue.

The number is arbitrary set to the number of fetch, but it could be lower or higher, it does not really matter. The idea was only to avoid having a blocking channel;

Before your change: non blocking with 7 slots
After your change: blocking with 1 slot

The channel is non-blocking for freeing go routines as soon as possible. This was more as a best practice rather than critical.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agreed, it is not an issue if the count is not up to date and avoids blocking at least up to this count, I will close this PR.

@arn-alt arn-alt closed this Oct 8, 2026
@arn-alt
arn-alt deleted the fix/fetch-failure-channel-size branch October 8, 2026 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants