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.

Sign up for GitHub

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

Jump to bottom

fix: proper lifecycle for following logs using consumers #366

Merged
merged 9 commits into from Mar 23, 2023
Merged

fix: proper lifecycle for following logs using consumers #366

merged 9 commits into from Mar 23, 2023

Conversation

slsyy
Copy link
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
Collaborator

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
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
Collaborator

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
Collaborator

@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

netlify bot commented Mar 20, 2023 •
edited

✅ 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
Collaborator

@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
Collaborator

@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

@sonarcloud
Copy link

sonarcloud 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
49 checks passed
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.

None yet
2 participants
@slsyy @mdelapenya