Skip to content

fix: proper lifecycle for following logs using consumers#366

Merged
mdelapenya merged 9 commits intotestcontainers:mainfrom
slsyy:follow_logs
Mar 23, 2023
Merged

fix: proper lifecycle for following logs using consumers#366
mdelapenya merged 9 commits intotestcontainers:mainfrom
slsyy:follow_logs

Conversation

@slsyy
Copy link
Copy Markdown
Contributor

@slsyy slsyy commented Oct 12, 2021

  • fix the doc: c.FollowOutput() MUST be called before
    c.StartLogProducer() due to date race
  • do not allow multiple c.StartLogProducer() without calling a
    c.StopLogProducer()
  • run c.StopLogProducer() in c.Terminate() to reduce risk of an
    accidental goroutine leak
  • fix tests, write new tests

fix: Following Container Logs feature fixes
65c2409 
* fix the doc: `c.FollowOutput()` MUST be called before
  `c.StartLogProducer()` due to date race
* do not allow multiple `c.StartLogProducer()` without calling a
  `c.StopLogProducer()`
* run `c.StopLogProducer()` in `c.Terminate()` to reduce risk of an
   accidental goroutine leak
* fix tests, write new tests
@mdelapenya
Copy link
Copy Markdown
Member

mdelapenya commented Oct 21, 2021

Hey @slsyy there is one failing test on CI: https://github.com/testcontainers/testcontainers-go/runs/3875582882?check_suite_focus=true

Can you check? 🙏

@slsyy
Copy link
Copy Markdown
Contributor Author

slsyy commented Oct 25, 2021

@mdelapenya I will try in the weekend. Perhaps problem is somewhere in the StartLogProducer. The logic is so weird, that I need to some time to analyse it: for example we reopen connection every 5 seconds due to usage of ctx, cancel := context.WithTimeout(ctx, time.Second*5), which is weird. Other problem, which I have found during testing in my private project is a CPU usage: StartLogProducer was responsible for ~90%, which also seems weird for such an ancillary operation

@mdelapenya
Copy link
Copy Markdown
Member

mdelapenya commented Oct 26, 2021

The logic is so weird, that I need to some time to analyse it: for example we reopen connection every 5 seconds due to usage of ctx, cancel := context.WithTimeout(ctx, time.Second*5), which is weird.

Interesting, maybe we can open a separate issue, or a discussion to improve that logic.

Other problem, which I have found during testing in my private project is a CPU usage: StartLogProducer was responsible for ~90%, which also seems weird for such an ancillary operation

Again interesting, could you please provide more info in an issue so we can trace and try to resolve it if possible? 🙏

@jakobmoellerdev jakobmoellerdev mentioned this pull request Mar 14, 2022
Merged
@mdelapenya
Copy link
Copy Markdown
Member

mdelapenya commented Oct 3, 2022

@slsyy I'd like to start the review of this PR but I see merge conflicts on it. Would you mind resolving it? 🙏 Thanks!

@mdelapenya
Merge branch 'main' into follow_logs
bcdcbd8 
* main: (335 commits)
  chore: reduce concurrent builds (testcontainers#702)
  chore: add mysql example (testcontainers#700)
  chore(deps): bump google.golang.org/api from 0.104.0 to 0.105.0 (testcontainers#699)
  chore(deps): bump google.golang.org/api in /examples/firestore (testcontainers#683)
  chore(deps): bump cloud.google.com/go/spanner in /examples/spanner (testcontainers#688)
  chore(deps): bump google.golang.org/api in /examples/pubsub (testcontainers#685)
  chore(deps): bump google.golang.org/api in /examples/spanner (testcontainers#684)
  chore(deps): bump google.golang.org/grpc in /examples/firestore (testcontainers#686)
  chore(deps): bump google.golang.org/api in /examples/bigtable (testcontainers#680)
  chore(deps): bump google.golang.org/api in /examples/datastore (testcontainers#678)
  chore(deps): bump golang.org/x/text from 0.3.7 to 0.5.0 (testcontainers#660)
  chore(deps): bump github.com/magiconair/properties from 1.8.6 to 1.8.7 (testcontainers#677)
  chore: postgres example (testcontainers#674)
  Add bigtable example (testcontainers#676)
  chore(deps): bump github.com/containerd/containerd from 1.6.10 to 1.6.12 (testcontainers#675)
  chore: run go mod tidy in examples (testcontainers#672)
  Improve datastore, firestore, pubsub and spanner tests (testcontainers#670)
  chore: group dependabot updates (testcontainers#668)
  chore: update mkdocs format to go-yaml v3 (testcontainers#667)
  chore: generate dependabot configs for examples (testcontainers#654)
  ...
@mdelapenya mdelapenya requested a review from a team as a code owner December 21, 2022 07:25
@netlify
Copy link
Copy Markdown

netlify bot commented Mar 20, 2023 •
edited
Loading

✅ Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 235797f
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/641b294e388b7f0008313d51
😎 Deploy Preview https://deploy-preview-366--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mdelapenya
Copy link
Copy Markdown
Member

mdelapenya commented Mar 20, 2023

@slsyy I went ahead and resolved the conflicts myself on top of your commits.

Do you mind adding a review to verify if your initial intention is not modified?

Thanks in advance

@mdelapenya mdelapenya mentioned this pull request Mar 21, 2023
Merged
@mdelapenya mdelapenya changed the title fix: Following Container Logs feature fixes fix: proper lifecycle for following logs using consumers Mar 22, 2023
@mdelapenya mdelapenya added the bug An issue with the library label Mar 22, 2023
@mdelapenya mdelapenya self-assigned this Mar 22, 2023
mdelapenya
mdelapenya approved these changes Mar 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Mar 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mdelapenya mdelapenya merged commit dd66783 into testcontainers:main Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdelapenya mdelapenya mdelapenya approved these changes

Assignees

@mdelapenya mdelapenya

Labels

bug An issue with the library

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@slsyy @mdelapenya