Skip to content

Conversation

mabaasit
Copy link
Collaborator

@mabaasit mabaasit commented Apr 21, 2022

feat(aggregation-count): count docs COMPASS-5756

count aggregation documents

Description

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

Comment on lines +46 to +47
case WorkspaceActionTypes.WorkspaceChanged:
return INITIAL_STATE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we are not cleaning up inflight counts when workspace type is changed (both for count and for results), you can run into a pretty nasty race condition where slower, older pipeline results and counts replace content of the newer one. Here's how it looks in UI:

Kapture 2022-04-21 at 18 16 46

We should definitely fix that both for counts and for results

Comment on lines +39 to +42
if (view === 'builder') {
dispatch(cancelAggregation());
dispatch(cancelCount());
}
Copy link
Collaborator

@gribnoysup gribnoysup Apr 25, 2022

Choose a reason for hiding this comment

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

We're definitely getting there. I think you would also want to do this when Run button is pressed if there is an in-flight request running, otherwise I can overload server pretty easily (but more importantly if I'm running a long query and maybe change collation or maxtimems there is a chance of running into race condition issue):

Kapture 2022-04-25 at 17 50 21

@mabaasit mabaasit requested a review from gribnoysup April 27, 2022 08:57
Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

I think you can improve the aggregation state slice a bit by not deriving state in the action and doing it in a reducer instead. Otherwise looks good

@mabaasit mabaasit merged commit 7c2b262 into main Apr 27, 2022
@mabaasit mabaasit deleted the COMPASS-5756-aggregation-count branch April 27, 2022 15:52
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