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

Remove the attemptId notion in the connectionManagerWorkflow #10780

Merged
merged 16 commits into from
Mar 10, 2022

Conversation

benmoriceau
Copy link
Contributor

@benmoriceau benmoriceau commented Mar 2, 2022

What

This is removing the attemptId from the create attempt activity to replace it with the attemptNumber. This will be modified in the workflow in a later commit.

@github-actions github-actions bot added area/platform issues related to the platform area/scheduler area/worker Related to worker labels Mar 2, 2022
@benmoriceau benmoriceau temporarily deployed to more-secrets March 2, 2022 00:13 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets March 2, 2022 00:13 Inactive
@benmoriceau benmoriceau requested review from cgardens and removed request for cgardens March 2, 2022 00:57
@benmoriceau benmoriceau temporarily deployed to more-secrets March 2, 2022 00:58 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets March 2, 2022 00:58 Inactive
@davinchia
Copy link
Contributor

Benoit, can you explain more why replacing the attempt id with the attempt number is better? Probably a dumb question, what's the difference between the attempt id and the attempt number?

@benmoriceau benmoriceau marked this pull request as ready for review March 2, 2022 18:10
@benmoriceau
Copy link
Contributor Author

Benoit, can you explain more why replacing the attempt id with the attempt number is better? Probably a dumb question, what's the difference between the attempt id and the attempt number?

It is to help understanding what represent the attemptId/Attempt number that we have in the temporal workflow. The doc in airbyte-scheduler/persistence/src/main/java/io/airbyte/scheduler/persistence/JobPersistence.java was missleading because the implementation is returning the attemptNumber instead of the attemptId. The goal is to remove the confusion because here attemptId we use in the temporal workflow is actually an attempt number.

Here is the persistence implementation (just the return that select the number):

return ctx.fetch(
          "INSERT INTO attempts(job_id, attempt_number, log_path, status, created_at, updated_at) VALUES(?, ?, ?, CAST(? AS ATTEMPT_STATUS), ?, ?) RETURNING attempt_number",
          jobId,
          job.getAttemptsCount(),
          logPath.toString(),
          Sqls.toSqlName(AttemptStatus.RUNNING),
          now,
          now)
          .stream()
          .findFirst()
          .map(r -> r.get("attempt_number", Integer.class))
          .orElseThrow(() -> new RuntimeException("This should not happen"));
    });

@benmoriceau benmoriceau temporarily deployed to more-secrets March 2, 2022 18:28 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets March 2, 2022 18:29 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets March 2, 2022 18:39 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets March 2, 2022 18:39 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets March 3, 2022 23:12 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets March 3, 2022 23:13 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets March 3, 2022 23:48 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets March 3, 2022 23:48 Inactive
@benmoriceau benmoriceau marked this pull request as draft March 4, 2022 00:00
@benmoriceau benmoriceau merged commit 99338c8 into master Mar 10, 2022
@benmoriceau benmoriceau deleted the bmoric/remove-attempt-id branch March 10, 2022 23:33
benmoriceau added a commit that referenced this pull request Mar 11, 2022
benmoriceau added a commit that referenced this pull request Mar 11, 2022
benmoriceau added a commit that referenced this pull request Mar 11, 2022
benmoriceau added a commit that referenced this pull request Mar 11, 2022
terencecho pushed a commit that referenced this pull request Mar 14, 2022
This is removing the attemptId from the create attempt activity to replace it with the attemptNumber. This will be modified in the workflow in a later commit.
terencecho pushed a commit that referenced this pull request Mar 14, 2022
terencecho pushed a commit that referenced this pull request Mar 14, 2022
terencecho pushed a commit that referenced this pull request Mar 14, 2022
This is removing the attemptId from the create attempt activity to replace it with the attemptNumber. This will be modified in the workflow in a later commit.
terencecho pushed a commit that referenced this pull request Mar 14, 2022
terencecho pushed a commit that referenced this pull request Mar 14, 2022
benmoriceau added a commit that referenced this pull request Mar 14, 2022
benmoriceau added a commit that referenced this pull request Mar 15, 2022
terencecho added a commit that referenced this pull request Mar 18, 2022
* Add Disable Failing Connections feature

* Rename and cleanup

* list jobs based off connection id

* Move variables to env config and update unit tests

* Fix env flag name

* Fix missing name changes

* Add comments to unit test

* Address PR comments

* Support multiple config types

* Update unit tests

* Remove the attemptId notion in the connectionManagerWorkflow (#10780)

This is removing the attemptId from the create attempt activity to replace it with the attemptNumber. This will be modified in the workflow in a later commit.

* Revert "Remove the attemptId notion in the connectionManagerWorkflow (#10780)" (#11057)

This reverts commit 99338c8.

* Revert "Revert "Remove the attemptId notion in the connectionManagerWorkflow (#10780)" (#11057)" (#11073)

This reverts commit 892dc7e.

* Revert "Revert "Revert "Remove the attemptId notion in the connectionManagerWorkflow (#10780)" (#11057)" (#11073)" (#11081)

This reverts commit e27bb74.

* Add Disable Failing Connections feature

* Rename and cleanup

* Fix rebase

* only disable if first job is older than max days

* Return boolean for activity

* Return boolean for activity

* Add unit tests for ConnectionManagerWorkflow

* Utilize object output for activity and ignore non success or failed runs

* Utilize object output for activity and ignore non success or failed runs

Co-authored-by: Benoit Moriceau <benoit@airbyte.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/scheduler area/worker Related to worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants