Temporal activity heartbeat from incorrect thread fix #11878

itaseskii · 2022-04-11T15:00:59Z

What

Fix activity heartbeats from incorrect thread as explained here #11515.

How

Retrieving the activity context directly in the activity implementation to avoid leaving the scope of the current thread.

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

┆Issue is synchronized with this Monday item by Unito

itaseskii · 2022-04-11T21:12:22Z

airbyte-workers/src/main/java/io/airbyte/workers/temporal/CancellationHandler.java

@@ -48,7 +48,7 @@ public void checkAndHandleCancellation(final Runnable onCancellationCallback) {
         * {@link TemporalUtils#withBackgroundHeartbeat} for where we actually send heartbeats to ensure
         * that we don't time out the activity.
         */
-        context.heartbeat(null);
+        activityContext.heartbeat(null);
      } catch (final ActivityCompletionException e) {


@jrhizor Since the cancellation handler is being called from scheduled background threads to initiate heartbeats I think that it should also handle generic exceptions in case there are some transient errors such as networking errors. The current implementation fails to do that which will lead to the scheduled executor service swallowing the exception and stopping all further work of the thread, including scheduling it again. Otherwise this case will need to be handled in the scheduleAtFixedRate() lambda definition.

benmoriceau · 2022-04-12T22:52:11Z

Hello @itaseskii, Thanks for your submission. It look good to me, there is many changes that seems to be format related. Could you run ./gradlew format?

Thanks,

marcosmarxm · 2022-04-14T00:16:39Z

@itaseskii are you able to build dev locally?

CLAassistant · 2022-04-15T10:53:15Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ itaseskii
❌ Ivica

Ivica seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

itaseskii · 2022-04-15T11:07:17Z

Hello @itaseskii, Thanks for your submission. It look good to me, there is many changes that seems to be format related. Could you run ./gradlew format?

Thanks,

@benmoriceau Done. Some reformats are intentional such as moving the lambdas in new lines when calling TemporalUtils.withBackgroundHeartbeat()

benmoriceau · 2022-04-19T20:08:23Z

Hello @itaseskii,
Sorry about the delay, this is the first community review I am doing and I am not so familiar with the process of it. The review looks good for me and I have trigger a build to test it.

Thanks again for your contribution!

benmoriceau · 2022-04-19T22:43:28Z

@itaseskii #12152 is green, I don't know what the issue is with the license agreement. Let me know if you want me to merge the PR if you don't manage to fix the licence issue.

itaseskii · 2022-04-20T12:11:52Z

@itaseskii #12152 is green, I don't know what the issue is with the license agreement. Let me know if you want me to merge the PR if you don't manage to fix the licence issue.

@benmoriceau I have already signed the agreement with my itaseskii git username. I was reinstalling my machine and I pushed some changes without configuring my git info and that is why it show that Ivica hasn't signed the agreement.

btw should we address this #11878 (comment) before merging? 🤔

benmoriceau · 2022-04-20T20:26:32Z

@benmoriceau I have already signed the agreement with my itaseskii git username. I was reinstalling my machine and I pushed some changes without configuring my git info and that is why it show that Ivica hasn't signed the agreement.

btw should we address this #11878 (comment) before merging? 🤔

I don't think it should block the merge, but this is something that we need to address. I'll create an issue for it.

itaseskii · 2022-04-20T20:34:43Z

@benmoriceau I have already signed the agreement with my itaseskii git username. I was reinstalling my machine and I pushed some changes without configuring my git info and that is why it show that Ivica hasn't signed the agreement.
btw should we address this #11878 (comment) before merging? thinking

I don't think it should block the merge, but this is something that we need to address. I'll create an issue for it.

@benmoriceau great. I am willing to fix it if that is okay with you :)

benmoriceau · 2022-04-26T21:56:43Z

@benmoriceau great. I am willing to fix it if that is okay with you :)

I have merge it. Thanks for willing to fix it!

itaseskii · 2022-04-26T22:03:07Z

@benmoriceau great. I am willing to fix it if that is okay with you :)

I have merge it. Thanks for willing to fix it!

Awesome. Have you maybe created an issue for it?

* fix activity context retrieval * add heartbeat unit tests * use cancelation handler for heartbeats * use await for blocking * format code Co-authored-by: Ivica <itaseski@debian-BULLSEYE-live-builder-AMD64>

fix activity context retrieval

ced0552

github-actions bot added area/platform issues related to the platform area/worker Related to worker labels Apr 11, 2022

octavia-squidington-iii added the community label Apr 11, 2022

jrhizor requested a review from benmoriceau April 11, 2022 16:49

add heartbeat unit tests

10a8f95

itaseskii closed this Apr 11, 2022

itaseskii reopened this Apr 11, 2022

use cancelation handler for heartbeats

8403d7d

itaseskii changed the title ~~Draft: Temporal activity heartbeat from incorrect thread fix~~ Temporal activity heartbeat from incorrect thread fix Apr 11, 2022

itaseskii commented Apr 11, 2022

View reviewed changes

itaseskii added 2 commits April 12, 2022 00:43

use await for blocking

a81042f

resolve conflicts

58fc17f

marcosmarxm assigned marcosmarxm and benmoriceau Apr 13, 2022

format code

34d8ad2

benmoriceau temporarily deployed to more-secrets April 19, 2022 20:07 Inactive

benmoriceau approved these changes Apr 19, 2022

View reviewed changes

benmoriceau merged commit f816946 into airbytehq:master Apr 26, 2022

octavia-squidington-iii mentioned this pull request Apr 27, 2022

Bump Airbyte version from 0.36.3-alpha to 0.36.4-alpha #12415

Merged

itaseskii mentioned this pull request May 4, 2022

🎉 Source Pipedrive: Add unit tests #11443

Closed

40 tasks

marcosmarxm mentioned this pull request Jun 14, 2022

Temporal activity heartbeat from incorrect thread #11515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temporal activity heartbeat from incorrect thread fix #11878

Temporal activity heartbeat from incorrect thread fix #11878

itaseskii commented Apr 11, 2022 •

edited by sync-by-unito bot

Loading

itaseskii Apr 11, 2022 •

edited

Loading

benmoriceau commented Apr 12, 2022

marcosmarxm commented Apr 14, 2022

CLAassistant commented Apr 15, 2022

itaseskii commented Apr 15, 2022

benmoriceau commented Apr 19, 2022

benmoriceau commented Apr 19, 2022

itaseskii commented Apr 20, 2022

benmoriceau commented Apr 20, 2022

itaseskii commented Apr 20, 2022

benmoriceau commented Apr 26, 2022

itaseskii commented Apr 26, 2022

Temporal activity heartbeat from incorrect thread fix #11878

Temporal activity heartbeat from incorrect thread fix #11878

Conversation

itaseskii commented Apr 11, 2022 • edited by sync-by-unito bot Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

itaseskii Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

benmoriceau commented Apr 12, 2022

marcosmarxm commented Apr 14, 2022

CLAassistant commented Apr 15, 2022

itaseskii commented Apr 15, 2022

benmoriceau commented Apr 19, 2022

benmoriceau commented Apr 19, 2022

itaseskii commented Apr 20, 2022

benmoriceau commented Apr 20, 2022

itaseskii commented Apr 20, 2022

benmoriceau commented Apr 26, 2022

itaseskii commented Apr 26, 2022

itaseskii commented Apr 11, 2022 •

edited by sync-by-unito bot

Loading

itaseskii Apr 11, 2022 •

edited

Loading