Skip to content

Conversation

@DavidS-ovm
Copy link
Contributor

No description provided.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@DavidS-ovm
Copy link
Contributor Author

All those alerts would be fixed by github/codeql#13835

Copy link
Member

@dylanratcliffe dylanratcliffe left a comment

Choose a reason for hiding this comment

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

Approved but with one comment

// fall through from all "final" query states, check if there's still queries in progress;
// only break from the loop if all queries have already been sent
// TODO: see above, still needs DefaultStartTimeout implemented to account for slow sources
allDone := allDone(ctx, activeQueries, lf)
Copy link
Member

Choose a reason for hiding this comment

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

Realistically this check shouldn't be required as the gateway shouldn't tell us we're done unless everything is done right? Can we change maybe the "all responders done, with unfinished queries" log to an error so it's easier to spot? Ideally if this every happens we'd know about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

allDone has a Debugf("query still active") for exactly that.

@DavidS-ovm DavidS-ovm merged commit 5679eeb into main Aug 22, 2023
@DavidS-ovm DavidS-ovm deleted the manual-change branch August 22, 2023 07:32
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.

3 participants