Remove the additional submit #10291
Assignees
Labels
kind/toil
Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc.
version:8.2.0-alpha1
Marks an issue as being completely or in parts released in 8.2.0-alpha1
version:8.2.0
Marks an issue as being completely or in parts released in 8.2.0
Milestone
Comments
Zelldon
added
the
kind/toil
10 tasks
zeebe-bors-camunda bot
added a commit
that referenced
this issue
Sep 15, 2022
10352: PI Modification: Reject direct termination of child process r=remcowesterhoud a=remcowesterhoud ## Description <!-- Please explain the changes you made here. --> It is possible to terminate a process instance using a modification command. This is done by adding terminate instructions for all of the active element instances of the process. When this happens the process will be fully terminated. However, it could be the case that a process is started with a call activity. This causes a complex situation. Terminating the child process would mean the parent process is "stuck". There is a few ways around this. 1. We could terminate the parent process. This could have unintended consequence for the user. 2. An incident is created on the parent process. The problem here is that the current incident logic makes it impossible to resolve these incidents. These could be worked around by doing another modification. That leaves the question why the user wouldn't modify the parent process in the first place. 3. We reject the command and advice the user to modify the parent process if the child process needs to be terminated. The option we are going for is option 3. When we detect that we are trying to terminate a process which is called by a parent process, the command will be rejected. The user received a message with the advice to modify the parent process instead. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10347 10355: Run ScheduledTasks during processing r=Zelldon a=Zelldon ## Description Removes the guarantee that scheduled Tasks are only executed after commit and only if no inProcessing is going on. This is not necessary, since all actions we want to execute after a commit are executed via PostcommitTasks which are added to the ProcessingResult. See related comment and discussion #9723 (comment) Future improvements: * We can create our own actor for the SchedulingService this would decouple better than scheduling and processing * We can remove the multiple scheduling in the SchedulingService #10291 <!-- Please explain the changes you made here. --> ## Related issues <!-- Which issues are closed by this PR or are related --> closes #9723 related #10272 Co-authored-by: Remco Westerhoud <remco@westerhoud.nl> Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
|
@Zelldon is this a potential regression? We put it into ready/upcoming so we clarify the urgency within the next two weeks. |
This was referenced Sep 16, 2022
zeebe-bors-camunda bot
added a commit
that referenced
this issue
Sep 21, 2022
10428: Remove the extra submit from ProcessingScheduleServiceImpl r=Zelldon a=Zelldon ## Description And another round! 😁 After #10414 and #10390 I think I found a good way to replace the additional Actor#submit from the production code and move it to where it is necessary in tests. As described here #10414 (comment) it is not easily possible to create a separate Actor for the ScheduleService, due to concurrency issues and further work we would need to do here. ## Details In this PR I iterate over the ProcessingScheduleServiceImpl and remove the StreamProcessorContext from the constructor. The service gets all necessary dependencies injected on the constructor (except the actor), mostly as suppliers. This allows to lazy load dependencies and reduce dependencies for tests. It allowed to write some more unit-test like tests. The service gets an open method to initialize the service with the actor control and create its own writer. This allows us to no longer share the writer with the StreamProcessor, since this might lead to issues especially if we now run in between (during processing phases). Due to the later initializing of the writers we reduce resources on followers, if the service is never scheduled we don't need to create a writer. When the StreamProcessor is in the closing phase it will close the scheduled service as well. <!-- Please explain the changes you made here. --> ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10414 closes #10390 closes #10291 Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
zeebe-bors-camunda bot
added a commit
that referenced
this issue
Sep 21, 2022
10418: Add monitor backup API endpoint r=npepinpe a=npepinpe
## Description
This PR extends the backup management endpoint to add a monitoring API under `GET /{id}` (e.g. if the actuator is at `http://localhost:9600/actuator/backups`, then `GET http://localhost:9600/actuator/backups/{id}`).
At the moment we use the internal types provided by the broker client as the response types, but in the future this will be replaced by the OpenAPI generated types. As such, I didn't think it was necessary to create user facing types and write some mapping code, since we will scrape it and replace it. However, I understand that we don't know when that will done, so I'm open to doing it anyway for safety.
The acceptance tests were updated (and renamed to better reflect what they do), and as they overlap with the old `BackupIT` tests, those were deleted.
## Related issues
closes #9902
10428: Remove the extra submit from ProcessingScheduleServiceImpl r=Zelldon a=Zelldon
## Description
And another round! 😁 After #10414 and #10390
I think I found a good way to replace the additional Actor#submit from the production code and move it to where it is necessary in tests.
As described here #10414 (comment) it is not easily possible to create a separate Actor for the ScheduleService, due to concurrency issues and further work we would need to do here.
## Details
In this PR I iterate over the ProcessingScheduleServiceImpl and remove the StreamProcessorContext from the constructor.
The service gets all necessary dependencies injected on the constructor (except the actor), mostly as suppliers. This allows to lazy load dependencies and reduce dependencies for tests. It allowed to write some more unit-test like tests.
The service gets an open method to initialize the service with the actor control and create its own writer. This allows us to no longer share the writer with the StreamProcessor, since this might lead to issues especially if we now run in between (during processing phases).
Due to the later initializing of the writers we reduce resources on followers, if the service is never scheduled we don't need to create a writer.
When the StreamProcessor is in the closing phase it will close the scheduled service as well.
<!-- Please explain the changes you made here. -->
## Related issues
<!-- Which issues are closed by this PR or are related -->
closes #10414
closes #10390
closes #10291
Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
10 tasks
zeebe-bors-camunda bot
added a commit
that referenced
this issue
Sep 30, 2022
10554: Remove actor#submit from ScheduleService r=Zelldon a=Zelldon ## Description After #10468 is merged we can open the PR for review. ### PR contains: * Use an own executor in the EngineRule for the exporting of the records, instead of the ProcessingScheduleService. The service should only be accessed by the same actor (Processing Actor). * Remove LogStream from context (finally) * Remove [remove ProcessingScheduleServiceIntegrationTest](88fb590) this is no longer possible to test like this, as written above the service should only accessed by the same actor. We have as replacement the ProcessingScheduleServiceTest which uses an own actor for the test. * Remove FINALLY the extra actor#submit <!-- Please explain the changes you made here. --> ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10291 10565: deps(parent): bump zeebe-test-container to use retry behavior r=oleschoenburg a=npepinpe ## Description Updates zeebe-test-container to 3.5.2, which includes retrying connection errors when exposing a host port in case of network issues with remove VMs. See camunda-community-hub/zeebe-test-container#354 ## Related issues closes #10287 10571: deps(maven): bump testcontainers-bom from 1.17.3 to 1.17.4 r=oleschoenburg a=dependabot[bot] Bumps [testcontainers-bom](https://github.com/testcontainers/testcontainers-java) from 1.17.3 to 1.17.4. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/testcontainers/testcontainers-java/releases">testcontainers-bom's releases</a>.</em></p> <blockquote> <h2>1.17.4</h2> <h1>What's Changed</h1> <h2>Highlights</h2> <p>This release has been made possible through the efforts of whopping 23 contributors, wow! 🤯</p> <p>Besides 3 new modules, this release brings a couple of bugfixes, improved compatibility and resilience in certain scenarios, better defaults and more configurability.</p> <p>You might also notice many PRs related to the documentation, templates for PRs and issues, and automation regarding OSS contributions. Testcontainers has always been a project with a lot of involvement by the community and we are very proud of this. That’s why want to make contributing to Testcontainers a great experience, no matter if you raise an issue, submit a PR or initiate a discussion in GitHhub Discussions.</p> <h3>🐼 New Module: Redpanda (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5740">#5740</a>) <a href="https://github.com/eddumelendez"><code>`@eddumelendez</code></a></h3>` <p><a href="https://redpanda.com/">Redpanda</a>, a Kafka-compatible streaming platform, recently added a special <code>dev-container</code> mode to their container image, that allows even faster startup times. A great reason to work in a Testcontainers module that leverages this flag by default to give you a great integration testing experience when using Redpanda. And of course, using Redpanda with Testcontainers is as easy and convenient as you are used to:</p> <pre><code>var container = new RedpandaContainer("docker.redpanda.com/vectorized/redpanda:v22.2.1") container.start() var connectionUrl = container.getBootstrapServers() // use the connectionUrl and start testing! </code></pre> <p>You can check out the <a href="https://www.testcontainers.org/modules/redpanda/">docs</a> to learn more.</p> <h3>New Module: TiDB (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5511">#5511</a>) <a href="https://github.com/Icemap"><code>`@Icemap</code></a></h3>` <p>With <a href="https://docs.pingcap.com/tidb/stable/overview">TiDB</a>, we are adding support for a new database module. As with other databases that can be accessed via JDBC, you can leverage Testcontainers’ special JDBC URL integration:</p> <pre><code>jdbc:tc:tidb:v6.1.0:///databasename </code></pre> <h3>New Module: Hashicorp Consul (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/4683">#4683</a>) <a href="https://github.com/julb"><code>`@julb</code></a></h3>` <p><a href="https://www.consul.io/">Consul</a></p> <h2>🚀 Features & Enhancements</h2> <ul> <li>getContainerByServiceName should work without suffix (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5776">#5776</a>) <a href="https://github.com/REslim30"><code>`@REslim30</code></a></li>` <li>Allow Pulsar default WaitStrategy to honour startup timeout (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5674">#5674</a>) <a href="https://github.com/nahguam"><code>`@nahguam</code></a></li>` <li>fix: ContainerDatabaseDriver does not register Properties object (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5829">#5829</a>) <a href="https://github.com/REslim30"><code>`@REslim30</code></a></li>` <li>couchbase: allow to configure bucket replicas and default to 0. (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5840">#5840</a>) <a href="https://github.com/daschl"><code>`@daschl</code></a></li>` <li>Add compatibility with MongoDB 6 (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5771">#5771</a>) <a href="https://github.com/eddumelendez"><code>`@eddumelendez</code></a></li>` <li>Set default elasticsearch heap size to 2GB (Alternate PR) (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5684">#5684</a>) <a href="https://github.com/REslim30"><code>`@REslim30</code></a></li>` <li>Add <code>Transferable.of(String, int)</code> (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5741">#5741</a>) <a href="https://github.com/eddumelendez"><code>`@eddumelendez</code></a></li>` <li>Make TestcontainersExtension public (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5285">#5285</a>) <a href="https://github.com/hmatt1"><code>`@hmatt1</code></a></li>` <li>Update Cassandra driver to 4.x (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/2830">#2830</a>) <a href="https://github.com/emerkle826"><code>`@emerkle826</code></a></li>` <li>Make outer maximum startup timeout in <code>DockerComposeContainer</code> configurable (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5588">#5588</a>) <a href="https://github.com/henri-tremblay"><code>`@henri-tremblay</code></a></li>` <li>Improve Pulsar's wait strategy to rely on clusterName (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5613">#5613</a>) <a href="https://github.com/eddumelendez"><code>`@eddumelendez</code></a></li>` </ul> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/2215e219054ee034583c27f3631154d7ec1b908e"><code>2215e21</code></a> Add Testcontainers icon for JetBrains IDEs (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5870">#5870</a>)</li> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/405ddb7e39443993f8988c4c5782727f0d0b5cc5"><code>405ddb7</code></a> Allow Pulsar default WaitStrategy to honour startup timeout (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5674">#5674</a>)</li> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/f54a29a48419b4fb14ef1fc024a656e918d994ae"><code>f54a29a</code></a> <code>getLivenessCheckPortNumbers()</code> should return mapped port (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5734">#5734</a>)</li> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/9847d5930bb0faea9d14f606c583919d4e2d2113"><code>9847d59</code></a> Fix: ContainerDatabaseDriver does not register Properties object (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5829">#5829</a>)</li> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/de1a77ed837ab5b5a542dbd51d40e08391edb129"><code>de1a77e</code></a> Improve consistency of Testcontainers name in docs (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5866">#5866</a>)</li> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/459d2f6b8915b9c0cbcd89b24825d412a6739838"><code>459d2f6</code></a> Use <code>testCompileOnly</code> instead of <code>testCompileClasspath</code> (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5849">#5849</a>)</li> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/22aa85d24e91a2a380da6d96f21ca03947de9c91"><code>22aa85d</code></a> Remove thundra from ci.yml (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5850">#5850</a>)</li> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/9e98addab9cfe076ace39a41c8cfdf4351649756"><code>9e98add</code></a> Update slf4j in test-support to 2.0.0 (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5848">#5848</a>)</li> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/9540652fa46f10f4d4e724787aa5fdf6ca3fbea6"><code>9540652</code></a> Update localstack images in tests (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5783">#5783</a>)</li> <li><a href="https://github.com/testcontainers/testcontainers-java/commit/1f3a1f764f5c31a2f5715ed502b417475aacdd98"><code>1f3a1f7</code></a> couchbase: allow to configure bucket replicas and default to 0. (<a href="https://github-redirect.dependabot.com/testcontainers/testcontainers-java/issues/5840">#5840</a>)</li> <li>Additional commits viewable in <a href="https://github.com/testcontainers/testcontainers-java/compare/1.17.3...1.17.4">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting ``@dependabot` rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - ``@dependabot` rebase` will rebase this PR - ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it - ``@dependabot` merge` will merge this PR after your CI passes on it - ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it - ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging - ``@dependabot` reopen` will reopen this PR if it is closed - ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> 10572: ci(gha): disable cache for short stages r=megglos a=megglos ## Description `Smoke tests` does not build all modules, thus sharing the cache with other bigger jobs introduces a race condition on who finishes first, thus the cache might be smaller than needed by other jobs. Furthermore jobs like client tests & Smoke tests are so short it's not worth using limited cache space for them. Co-authored-by: Christopher Zell <zelldon91@googlemail.com> Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Meggle (Sebastian Bathke) <sebastian.bathke@camunda.com>
10 tasks
zeebe-bors-camunda bot
added a commit
that referenced
this issue
Sep 30, 2022
10584: [Backport release-8.1.0]: Remove actor#submit from ScheduleService r=oleschoenburg a=Zelldon ## Description Backports #10554 to the release branch, since I think it makes sense to have in the release as well. Afterwards I have to recreate the benchmarks and rerun the e2e tests. <!-- Please explain the changes you made here. --> I had to do cherry-pick manually since there were conflicts with the ProcessingScheduleServiceIntegrationTest (we delete them in this PR). No other commit was harmed. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10291 Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
zeebe-bors-camunda bot
added a commit
that referenced
this issue
Sep 30, 2022
10584: [Backport release-8.1.0]: Remove actor#submit from ScheduleService r=Zelldon a=Zelldon ## Description Backports #10554 to the release branch, since I think it makes sense to have in the release as well. Afterwards I have to recreate the benchmarks and rerun the e2e tests. <!-- Please explain the changes you made here. --> I had to do cherry-pick manually since there were conflicts with the ProcessingScheduleServiceIntegrationTest (we delete them in this PR). No other commit was harmed. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10291 Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
zeebe-bors-camunda bot
added a commit
that referenced
this issue
Sep 30, 2022
10584: [Backport release-8.1.0]: Remove actor#submit from ScheduleService r=Zelldon a=Zelldon ## Description Backports #10554 to the release branch, since I think it makes sense to have in the release as well. Afterwards I have to recreate the benchmarks and rerun the e2e tests. <!-- Please explain the changes you made here. --> I had to do cherry-pick manually since there were conflicts with the ProcessingScheduleServiceIntegrationTest (we delete them in this PR). No other commit was harmed. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10291 Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
korthout
added
the
version:8.2.0-alpha1
npepinpe
added
the
version:8.2.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
In the ProcessingScheduleServiceImpl we have a method, which submits a task to our scheduling queue (before it will be really submitted, like for run delayed).
This is somehow unexpected and can cause race conditions (it will slow down the execution). It looks like it was added only for testability f99a9a4 But we can achieve this also differently, by using an Actor in our tests.
The text was updated successfully, but these errors were encountered: