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

feat(): Added JDBC heartbeat #2081

Merged
merged 15 commits into from
Sep 28, 2023
Merged

feat(): Added JDBC heartbeat #2081

merged 15 commits into from
Sep 28, 2023

Conversation

Skraye
Copy link
Member

@Skraye Skraye commented Sep 11, 2023

close #2055

@Skraye Skraye requested a review from loicmathieu September 11, 2023 12:37
@Skraye Skraye self-assigned this Sep 11, 2023
loicmathieu
loicmathieu requested changes Sep 11, 2023
View reviewed changes
Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a log of business rules inside the repository, I wonder if it should not be moved to the caller as a repository should only take care of accessing the data.

core/src/main/java/io/kestra/core/runners/Worker.java Outdated Show resolved Hide resolved
core/src/main/java/io/kestra/core/runners/WorkerHeartbeat.java Outdated Show resolved Hide resolved
jdbc-postgres/src/main/resources/migrations/postgres/V2_1__initial.sql Outdated Show resolved Hide resolved
jdbc-postgres/src/main/resources/migrations/postgres/V2_1__initial.sql Outdated Show resolved Hide resolved
jdbc-postgres/src/main/resources/migrations/postgres/V2_1__initial.sql Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/repository/AbstractJdbcWorkerHeartbeatRepository.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/repository/AbstractJdbcWorkerHeartbeatRepository.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/runner/JdbcHeartbeat.java Outdated Show resolved Hide resolved
webserver/src/main/java/io/kestra/webserver/controllers/WorkerInstanceController.java Show resolved Hide resolved
cli/src/main/resources/application.yml Show resolved Hide resolved
@Skraye Skraye force-pushed the feat/jdbc-heartbeat branch 3 times, most recently from 748de53 to 8e9caeb Compare September 18, 2023 09:49
@Skraye
Copy link
Member Author

Skraye commented Sep 18, 2023

Still need to write tests, but will do it after the logic is validated.

@Skraye Skraye requested a review from loicmathieu September 18, 2023 09:55
@Skraye Skraye force-pushed the feat/jdbc-heartbeat branch from 8e9caeb to 848d933 Compare September 18, 2023 12:49
loicmathieu
loicmathieu requested changes Sep 19, 2023
View reviewed changes
Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't add time to review everything but here are a first batch of feedbacks.

As a side note, I would have prefer to have the PR split in two heartbeat and resubmit it would have been easier to reason about.

cli/src/main/resources/application.yml Outdated Show resolved Hide resolved
jdbc-h2/src/main/resources/migrations/h2/V1_2__initial.sql Outdated Show resolved Hide resolved
webserver/src/main/java/io/kestra/webserver/controllers/MiscController.java Outdated Show resolved Hide resolved
webserver/src/main/java/io/kestra/webserver/controllers/WorkerInstanceController.java Show resolved Hide resolved
core/src/main/java/io/kestra/core/repositories/WorkerInstanceRepositoryInterface.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/repository/AbstractJdbcWorkerJobRunningRepository.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/repository/AbstractJdbcWorkerJobRunningRepository.java Outdated Show resolved Hide resolved
jdbc-h2/src/main/java/io/kestra/runner/h2/H2WorkerJobQueue.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/runner/JdbcExecutor.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/runner/JdbcHeartbeat.java Outdated Show resolved Hide resolved
@Skraye Skraye force-pushed the feat/jdbc-heartbeat branch 3 times, most recently from 2a3d294 to a1f8ec5 Compare September 26, 2023 08:22
@Skraye Skraye marked this pull request as ready for review September 26, 2023 08:37
@Skraye Skraye mentioned this pull request Sep 26, 2023
Merged
@Skraye Skraye force-pushed the feat/jdbc-heartbeat branch 2 times, most recently from 9d7b607 to 251187a Compare September 26, 2023 12:56
@tchiotludo tchiotludo force-pushed the feat/jdbc-heartbeat branch 4 times, most recently from c423c91 to e0af053 Compare September 27, 2023 09:35
loicmathieu
loicmathieu requested changes Sep 27, 2023
View reviewed changes
core/src/main/java/io/kestra/core/repositories/WorkerJobRunningInterface.java Outdated Show resolved Hide resolved
core/src/test/java/io/kestra/core/repositories/AbstractWorkerInstanceRepositoryTest.java Outdated Show resolved Hide resolved
ui/src/translations.json Outdated Show resolved Hide resolved
ui/src/translations.json Outdated Show resolved Hide resolved
jdbc-h2/src/main/java/io/kestra/runner/h2/H2WorkerJobQueue.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/repository/AbstractJdbcWorkerJobRunningRepository.java Outdated Show resolved Hide resolved
jdbc-h2/src/main/resources/migrations/h2/V1_2__worker_heartbeat.sql Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/runner/JdbcExecutor.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/runner/JdbcHeartbeat.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/runner/JdbcHeartbeat.java Show resolved Hide resolved
@Skraye Skraye force-pushed the feat/jdbc-heartbeat branch 3 times, most recently from eaceb06 to 5f8c24a Compare September 27, 2023 14:20
@Skraye Skraye force-pushed the feat/jdbc-heartbeat branch from 5f8c24a to d475142 Compare September 27, 2023 14:24
loicmathieu
loicmathieu approved these changes Sep 27, 2023
View reviewed changes
Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@loicmathieu
Copy link
Member

See #2201 for some followup on the implementation

@Skraye Skraye force-pushed the feat/jdbc-heartbeat branch from d169037 to a70f840 Compare September 27, 2023 16:17
@Skraye Skraye force-pushed the feat/jdbc-heartbeat branch from 64ee18a to f3e8f61 Compare September 28, 2023 09:02
loicmathieu
loicmathieu approved these changes Sep 28, 2023
View reviewed changes
Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor feedback but overall LGTM

jdbc/src/main/java/io/kestra/jdbc/JdbcWorkerJobQueueService.java Outdated Show resolved Hide resolved
jdbc/src/main/java/io/kestra/jdbc/JdbcWorkerJobQueueService.java Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 16 Code Smells

72.2% 72.2% Coverage
2.3% 2.3% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@tchiotludo tchiotludo merged commit c9e6145 into develop Sep 28, 2023
@tchiotludo tchiotludo deleted the feat/jdbc-heartbeat branch September 28, 2023 12:36
@tchiotludo tchiotludo mentioned this pull request Oct 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchiotludo tchiotludo tchiotludo left review comments

@loicmathieu loicmathieu loicmathieu approved these changes

Assignees

@Skraye Skraye

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Zombie Worker Task Detection & Automatic Resubmission
3 participants
@Skraye @loicmathieu @tchiotludo