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

[CORE-2238] Loki in testpachd fast-follow #9913

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

robert-uhl
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.05%. Comparing base (653b179) to head (fd7e870).

❗ Current head fd7e870 differs from pull request most recent head 11530c9. Consider uploading reports for the commit 11530c9 to get more accurate results

Additional details and impacted files
@@                           Coverage Diff                            @@
##           jonathan/core-2238-loki-in-testpachd    #9913      +/-   ##
========================================================================
- Coverage                                 58.05%   58.05%   -0.01%     
========================================================================
  Files                                       609      609              
  Lines                                     74034    74034              
========================================================================
- Hits                                      42982    42978       -4     
- Misses                                    30499    30508       +9     
+ Partials                                    553      548       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ctx = pctx.Background("cleanup")
if err := clean.Cleanup(ctx); err != nil {
ctx := pctx.Background("cleanup")
if err = clean.Cleanup(ctx); err != nil {
Copy link
Contributor Author

@robert-uhl robert-uhl Apr 3, 2024

Choose a reason for hiding this comment

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

I think an error when cleaning up is probably worth not deleting the batch logs? That way it can be debugged.

// errors cause this, as does SIGINT.
<-ctx.Done()
if err := <-errCh; err != nil {
// wait for pachd to complete
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There’s no need to wait for context cancellation, since the run goroutine will close errCh when it completes.

@robert-uhl robert-uhl marked this pull request as ready for review April 3, 2024 14:49
@robert-uhl robert-uhl requested a review from jrockway April 3, 2024 14:50
@robert-uhl robert-uhl marked this pull request as draft April 3, 2024 15:04
@robert-uhl robert-uhl changed the title Unify context, wait on completion [CORE-2238] Loki in testpachd fast-follow Apr 4, 2024
@jrockway jrockway force-pushed the jonathan/core-2238-loki-in-testpachd branch from 37b4b6f to f02a218 Compare April 5, 2024 02:50
Base automatically changed from jonathan/core-2238-loki-in-testpachd to master April 5, 2024 03:27
@jrockway jrockway removed their request for review April 25, 2024 16:50
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.

None yet

2 participants