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

Fix aggregator to include timestamps of any results produced by a query that times out #11

Merged

Conversation

RubenEschauzier
Copy link
Contributor

This PR ensures the timestamps produced by query replications that time out or produce any other error are included in the results. The behavior of the aggregator is (should be) as follows:

  1. If a query times out or errors and produces no results, the replication is not included in any of the computed statistics. The aggregator only registers the error produced by the query and the number of failures. (Unchanged from current version)
  2. If a query times out or errors and produces some results, the timestamps are recorded and included in any computed averages, minimums, or maxes. The aggregator does not record the total query execution time or the number of results and any associated statistics like min or max.
  3. If a query completes, all statistics are aggregated, and a hash is recorded. This hash will be compared to all other successful query replications that are being aggregated. (Unchanged from the current version)

Tests were adjusted to reflect the change in behavior in the second case.

@rubensworks rubensworks requested a review from surilindur June 5, 2024 08:28
@coveralls
Copy link

coveralls commented Jun 5, 2024

Pull Request Test Coverage Report for Build 9380772123

Details

  • 36 of 36 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 9088210330: 0.0%
Covered Lines: 247
Relevant Lines: 247

💛 - Coveralls

lib/ResultAggregator.ts Outdated Show resolved Hide resolved
@surilindur
Copy link
Contributor

Besides that small comment, I cannot think of any other feedback on this. I think the change makes sense, because the results are only actually valid when all the executions complete without errors. When there is an error, I do not have strong opinions on what should be reported, as long as the error itself is there (which it is). 🙂

lib/ResultAggregator.ts Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 5, 2024

Pull Request Test Coverage Report for Build 9383473863

Details

  • 36 of 36 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 9088210330: 0.0%
Covered Lines: 247
Relevant Lines: 247

💛 - Coveralls

@rubensworks rubensworks merged commit 53cd567 into comunica:master Jun 5, 2024
6 checks passed
@rubensworks
Copy link
Member

Thanks! Released as 4.1.0.

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.

4 participants