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

Implement monitor backup api in gateway #9902

Closed
1 of 2 tasks
Tracked by #9606
deepthidevaki opened this issue Jul 27, 2022 · 0 comments · Fixed by #10418
Closed
1 of 2 tasks
Tracked by #9606

Implement monitor backup api in gateway #9902

deepthidevaki opened this issue Jul 27, 2022 · 0 comments · Fixed by #10418
Assignees
Labels
version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0

Comments

@deepthidevaki
Copy link
Contributor

deepthidevaki commented Jul 27, 2022

Depends on #9807

  • Define user interface
  • Gateway sends backup status query to all partitions
  • Gateway aggregates responses from all partitions and send the response to the user

Tasks:

@npepinpe npepinpe self-assigned this Sep 20, 2022
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>
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 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.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 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
zeebe-bors-camunda bot added a commit that referenced this issue Sep 22, 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 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
zeebe-bors-camunda bot added a commit that referenced this issue Sep 22, 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 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
zeebe-bors-camunda bot added a commit that referenced this issue Sep 22, 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 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
zeebe-bors-camunda bot added a commit that referenced this issue Sep 22, 2022
10412: Add a standalone app that can be used to restore a broker from a backup r=oleschoenburg a=deepthidevaki

## Description

* Added a new module for restore. 
* Adds a RestoreManager that coordinates restoring of all partitions. It first determines which partitions to restore from based on the given `BrokerCfg`. This configuration must be the same one used by the Broker when it is started after the restore. Because of this, restore module depens on broker module. But since it needs access to the configuration and `RaftPartitionGroupFactory`, there is no other way. There is scope of splitting broker to take these shared configs out of it.
* Added a `RestoreApp` in dist. My plan was to add it in restore module. But it needs access to BrokerConfig, which is built in dist. So the app is added to dist module. Alternative is to copy components `BrokerConfiguration` and `WorkerDirectory` to restore module. 
* The backup id to restore from is passed as a command line argument `--backupId=x`. This is parsed by spring boot. I decided to keep it simple for now, since we are planning to release it as experimental feature anyway. If we decide to support this app long term, then I suggest to switch to something like picocli that we use in zdb.

A basic test to verify a broker can be restored is added. More scenarios should be added later.

PS:- App is not added to the distribution yet.

## Related issues

related #10263 



10418: Add monitor backup API endpoint r=oleschoenburg 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 



10443: Do not take a backup if it already exists r=oleschoenburg a=deepthidevaki

## Description

After restore, the log is truncated to the checkpoint position. So the checkpoint record is processed again and will trigger a new backup with the same Id of the backup it restored from. With this PR, `BackupService` handles this case gracefully. In addition, we also do not take a new backup if existing backup is failed or in progress. Alternatively, we can delete this backup and take a new one. But chances of it happening (i.e triggering a new backup when one already is in progress/failed) is very low. So we can keep this simple.

## Related issues

closes #10430 



Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
zeebe-bors-camunda bot added a commit that referenced this issue Sep 22, 2022
10418: Add monitor backup API endpoint r=oleschoenburg 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 



10443: Do not take a backup if it already exists r=oleschoenburg a=deepthidevaki

## Description

After restore, the log is truncated to the checkpoint position. So the checkpoint record is processed again and will trigger a new backup with the same Id of the backup it restored from. With this PR, `BackupService` handles this case gracefully. In addition, we also do not take a new backup if existing backup is failed or in progress. Alternatively, we can delete this backup and take a new one. But chances of it happening (i.e triggering a new backup when one already is in progress/failed) is very low. So we can keep this simple.

## Related issues

closes #10430 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
zeebe-bors-camunda bot added a commit that referenced this issue Sep 22, 2022
10418: Add monitor backup API endpoint r=oleschoenburg 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 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
@Zelldon Zelldon added the version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0 label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants