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

Log producer start stop sync #1701

Merged

Conversation

gflarity
Copy link
Contributor

What does this PR do?

As part of trying to fix the issues discuss in #1700, I tried this first. Turns out the lack of sychronization in StartLogProducer and StopLogProducer was actually red herring, however it seems like is still a useful addition so I thought I'd send out the PR for review.

Why is it important?

Makes (Start/Stop)LogProducer more deterministic and perhaps will prevent future race condition bugs.

@gflarity gflarity requested a review from a team as a code owner September 29, 2023 21:11
@netlify
Copy link

netlify bot commented Sep 29, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit ba4566b
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/652d4df695059b00078d729e
😎 Deploy Preview https://deploy-preview-1701--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 configuration.

mdelapenya
mdelapenya previously approved these changes Oct 2, 2023
Copy link
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.

Let's wait until #1700 is merged to rebase this one, as they share the changeset.

Other than that, LGTM, thanks!

@mdelapenya
Copy link
Member

Oh well, this PR needs to be formatted. Could you please run make lint from the top of the project? 🙏

@mdelapenya
Copy link
Member

@gflarity since #1700 was already merge, I think we can work on simplifying this PR. Could you take a look please? 🙏

@gflarity
Copy link
Contributor Author

gflarity commented Oct 16, 2023

@gflarity since #1700 was already merge, I think we can work on simplifying this PR. Could you take a look please? 🙏

Yup, I've just rebased on main. Will update PR after tests pass :)

Update: Done. @mdelapenya PTAL.

Copy link
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, thanks!

@mdelapenya mdelapenya self-assigned this Oct 18, 2023
@mdelapenya mdelapenya added chore Changes that do not impact the existing functionality hacktoberfest Pull Requests accepted for Hacktoberfest. labels Oct 18, 2023
@mdelapenya mdelapenya merged commit d43fae3 into testcontainers:main Oct 24, 2023
114 checks passed
mdelapenya added a commit that referenced this pull request Oct 26, 2023
* main: (27 commits)
  docs: remove OpenSSF scorecard (#1823)
  Auto-cleanup of k6 build cache (#1788)
  Add OpenSSF Scorecards GitHub Action (#1795)
  chore(deps): bump google.golang.org/grpc from 1.57.0 to 1.57.1 (#1822)
  chore: expose SessionID (#1793)
  chore: use HTTP calls to invoke the lambda from the tests (#1794)
  wait for log producer to really stop inside StopLogProducer func (#1701)
  chore(deps): bump github.com/nats-io/nats-server/v2 in /modules/nats (#1784)
  chore(deps): bump urllib3 from 2.0.6 to 2.0.7 (#1781)
  chore: add an example of using localstack alongside AWS lambdas (#1790)
  chore(deps): combine and bump compose dependencies (#1787)
  feat: support for replacing images with custom substitutions (#1719)
  fix: data race in docker client `Info()` (#1779)
  Use correct formatting directive for errors in lifecycle logs (#1780)
  chore(deps): bump golang.org/x/mod from 0.12.0 to 0.13.0 in /modules/{elasticsearch,kafka} and /modulegen (#1778)
  chore(deps): bump github.com/rabbitmq/amqp091-go in /modules/rabbitmq (#1728)
  chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (#1732)
  ignore patterns defined in dockerignore (#1725)
  Fix wrong module names (#1776)
  docs: add default options to k6 module (#1744)
  ...
mdelapenya added a commit that referenced this pull request Oct 26, 2023
…ers/image-spec-1.1.0-rc5

* main: (49 commits)
  docs: remove OpenSSF scorecard (#1823)
  Auto-cleanup of k6 build cache (#1788)
  Add OpenSSF Scorecards GitHub Action (#1795)
  chore(deps): bump google.golang.org/grpc from 1.57.0 to 1.57.1 (#1822)
  chore: expose SessionID (#1793)
  chore: use HTTP calls to invoke the lambda from the tests (#1794)
  wait for log producer to really stop inside StopLogProducer func (#1701)
  chore(deps): bump github.com/nats-io/nats-server/v2 in /modules/nats (#1784)
  chore(deps): bump urllib3 from 2.0.6 to 2.0.7 (#1781)
  chore: add an example of using localstack alongside AWS lambdas (#1790)
  chore(deps): combine and bump compose dependencies (#1787)
  feat: support for replacing images with custom substitutions (#1719)
  fix: data race in docker client `Info()` (#1779)
  Use correct formatting directive for errors in lifecycle logs (#1780)
  chore(deps): bump golang.org/x/mod from 0.12.0 to 0.13.0 in /modules/{elasticsearch,kafka} and /modulegen (#1778)
  chore(deps): bump github.com/rabbitmq/amqp091-go in /modules/rabbitmq (#1728)
  chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (#1732)
  ignore patterns defined in dockerignore (#1725)
  Fix wrong module names (#1776)
  docs: add default options to k6 module (#1744)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality hacktoberfest Pull Requests accepted for Hacktoberfest.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants