chore: add mau partition deleter

[masaaania]

• This PR adds a job to delete the mau table's partition and fix the mau summarizer's behavior.

[masaaania]

/review

[github-actions]

PR Analysis

Main theme

type: Refactoring
description: Update the cronjob.yaml template to include a new cron job for mau-partition-deleter.

PR summary

type: Refactoring
description: This PR adds a new cron job to the cronjob.yaml template file called "mau-partition-deleter" and updates the values.yaml file to include the schedule for this new job.

Type of PR

Refactoring

PR Feedback:

General suggestions

No general suggestions.

Code feedback

relevant file: manifests/bucketeer/charts/batch/templates/cronjob.yaml

suggestion:

The --cacert option in the curl command on lines 661 and 727 should be removed as it's not being used. This will simplify the command and avoid confusion. [important]

relevant line:

-RES=`curl -X POST --cacert /usr/local/certs/service/tls.crt -d '{"job": "MauSummarizer"}' -H "authorization: bearer ${TOKEN}" -H "Content-Type: application/json" -s -o /dev/null -w '%{http_code}\\n' ${ENDPOINT}`
+RES=`curl -X POST -m 3600 --cacert /usr/local/certs/service/tls.crt -d '{"job": "MauSummarizer"}' -H "authorization: bearer ${TOKEN}" -H "Content-Type: application/json" -s -o /dev/null -w '%{http_code}\\n' ${ENDPOINT}`

suggestion:

The restartPolicy field on lines 656 and 724 should be removed as it is already set to Never by default. [medium]

relevant line:

-restartPolicy: Never
+```

#### suggestion:
The new cron job "mau-partition-deleter" could be written in a separate template file for better modularity and reusability. This way, each cron job template can be managed independently. [medium]

#### relevant file: manifests/bucketeer/charts/batch/values.yaml
#### suggestion:
Since the `mauPartitionDeleterSchedule` value is added in the `values.yaml` file, it would be helpful to provide a brief description for this new schedule to clarify its purpose. [medium]

#### relevant line: 
```diff
+  mauPartitionDeleterSchedule: "0 2 1 * *"

Security concerns:

No

[github-actions]

PR Analysis

Main theme

Bug fix

PR summary

This PR fixes an issue related to the batchDescriptor field in the values.yaml file of the web-gateway chart. The updated code resolves the issue by introducing a new batch descriptor value.

Type of PR

Bug fix

PR Feedback:

General suggestions

The changes made in this PR appear to address the reported bug successfully.

Code feedback

manifests/bucketeer/charts/web-gateway/values.yaml

• batchDescriptor: CsMEChlwcm90by9iYXRjaC9zZXJ2aWNlLnByb3RvEg9idWNrZXRlZXIuYmF0Y2giPgoPQmF0Y2hKb2JSZXF1ZXN0EisKA2pvYhgBIAEoDjIZLmJ1Y2tldGVlci5iYXRjaC5CYXRjaEpvYlIDam9iIhIKEEJhdGNoSm9iUmVzcG9uc2UqnQIKCEJhdGNoSm9iEhsKF0V4cGVyaW1lbnRTdGF0dXNVcGRhdGVyEAASHAoYRXhwZXJpbWVudFJ1bm5pbmdXYXRjaGVyEAESFwoTRmVhdHVyZVN0YWxlV2F0Y2hlchACEhMKD01hdUNvdW50V2F0Y2hlchADEhMKD0RhdGV0aW1lV2F0Y2hlchAEEhUKEUV2ZW50Q291bnRXYXRjaGVyEAUSFwoTRG9tYWluRXZlbnRJbmZvcm1lchAGEhcKE1JlZGlzQ291bnRlckRlbGV0ZXIQBxIdChlQcm9ncmVzc2l2ZVJvbGxvdXRXYXRjaGVyEAgSGAoURXhwZXJpbWVudENhbGN1bGF0b3IQCRIRCg1NYXVTdW1tYXJpemVyEAoyaAoMQmF0Y2hTZXJ2aWNlElgKD0V4ZWN1dGVCYXRjaEpvYhIgLmJ1Y2tldGVlci5iYXRjaC5CYXRjaEpvYlJlcXVlc3QaIS5idWNrZXRlZXIuYmF0Y2guQmF0Y2hKb2JSZXNwb25zZSIAQi9aLWdpdGh1Yi5jb20vYnVja2V0ZWVyLWlvL2J1Y2tldGVlci9wcm90by9iYXRjaGIGcHJvdG8z

• batchDescriptor: CtwEChlwcm90by9iYXRjaC9zZXJ2aWNlLnByb3RvEg9idWNrZXRlZXIuYmF0Y2giPgoPQmF0Y2hKb2JSZXF1ZXN0EisKA2pvYhgBIAEoDjIZLmJ1Y2tldGVlci5iYXRjaC5CYXRjaEpvYlIDam9iIhIKEEJhdGNoSm9iUmVzcG9uc2UqtgIKCEJhdGNoSm9iEhsKF0V4cGVyaW1lbnRTdGF0dXNVcGRhdGVyEAASHAoYRXhwZXJpbWVudFJ1bm5pbmdXYXRjaGVyEAESFwoTRmVhdHVyZVN0YWxlV2F0Y2hlchACEhMKD01hdUNvdW50V2F0Y2hlchADEhMKD0RhdGV0aW1lV2F0Y2hlchAEEhUKEUV2ZW50Q291bnRXYXRjaGVyEAUSFwoTRG9tYWluRXZlbnRJbmZvcm1lchAGEhcKE1JlZGlzQ291bnRlckRlbGV0ZXIQBxIdChlQcm9ncmVzc2l2ZVJvbGxvdXRXYXRjaGVyEAgSGAoURXhwZXJpbWVudENhbGN1bGF0b3IQCRIRCg1NYXVTdW1tYXJpemVyEAoSFwoTTWF1UGFydGl0aW9uRGVsZXRlchALMmgKDEJhdGNoU2VydmljZRJYCg9FeGVjdXRlQmF0Y2hKb2ISIC5idWNrZXRlZXIuYmF0Y2guQmF0Y2hKb2JSZXF1ZXN0GiEuYnVja2V0ZWVyLmJhdGNoLkJhdGNoSm9iUmVzcG9uc2UiAEIvWi1naXRodWIuY29tL2J1Y2tldGVlci1pby9idWNrZXRlZXIvcHJvdG8vYmF0Y2hiBnByb3RvMw==

The changes in the values.yaml file look good and seem to have resolved the issue. No further suggestions or improvements are needed.

Security concerns:

No, this PR does not introduce any security concerns.

[github-actions]

PR Analysis

Main theme

The main theme of this PR is to add a new job called mauPartitionDeleter for deleting old records from the MAU (Monthly Active Users) table in the database.

PR summary

This PR adds a new job called mauPartitionDeleter to the batch API in order to delete old records from the MAU table. It also adds the necessary code to configure and run the job in the server.

Type of PR

Enhancement

PR Feedback:

General suggestions

It's great to see the addition of a new job to handle the deletion of old records from the MAU table. The code looks good overall, and the changes seem to be well-contained. One improvement that could be made is to add some comments or documentation to describe the purpose and functionality of the mauPartitionDeleter job.

Code feedback

pkg/batch/api/api.go

• Line 63: The mauPartitionDeleter job should be added to the NewBatchService function as an argument and assigned to the mauPartitionDeleter field of the batchService struct.

pkg/batch/api/api_test.go

• Line 635: The mauPartitionDeleter job should be added to the newBatchService function as an argument.

pkg/batch/cmd/server/server.go

• Line 395: The mauPartitionDeleter job should be added to the Run method as an argument.

pkg/batch/jobs/mau/mau_partition_deleter.go

• Lines 23-26: Consider adding more comments to explain the purpose of each imported package.
• Line 28: The constant partitionRetentionMonths should have a comment explaining its meaning and purpose.
• Lines 40-44: Consider adding a comment block to explain the purpose and functionality of the NewMAUPartitionDeleter function.
• Line 46: Use the opts parameter directly instead of assigning it to dopts.
• Lines 55-57: Consider adding a comment block to explain the purpose and functionality of the Run method.
• Lines 60-66: Consider adding comments to each step in the deletion process to explain what it does.
• Lines 102-103: Consider adding comments to the getDeleteTargetPartition and newPartitionName functions to explain their purpose and functionality.

Security concerns:

No

[github-actions]

PR Analysis

Main theme

The main theme of this PR is to add test coverage for the mau_partition_deleter file, as well as adding new code to several files related to the mau package.

PR summary

This PR adds test coverage for the mau_partition_deleter file and introduces new code related to the mau package in several files.

Type of PR

Refactoring

PR Feedback:

General suggestions

The code changes look good and have been well-structured. The new code for the test file provides good coverage for the mau_partition_deleter file. However, it would be a good practice to add more descriptive test names to improve readability and maintainability. Additionally, consider adding test cases to cover edge cases and error scenarios.

Code feedback

mau_partition_deleter_test.go

• Line 99: Consider adding a test case with an empty desc string.

mau_summarizer.go

• Line 46: Consider adding a comment to describe the purpose of the log statement.

mau_summary.go

• Line 66: Consider adding a comment to describe the purpose of the is_finished field.

mau.go

• Lines 42-79: Consider adding error handling for the SQL queries. Currently, the errors are simply returned without any handling, which might lead to unexpected behavior.

[github-actions]

PR Analysis

Main theme

Bug fix

PR summary

This PR introduces new test cases for the MAUStorage implementation in the pkg/mau/storage/ directory. The tests cover the DeleteRecords, RebuildPartition, and DropPartition methods. Additionally, a new mock package mock/mau is added to mock the MAUStorage interface.

Type of PR

Bug fix

PR Feedback:

General suggestions

The test cases added in this PR seem to cover the necessary scenarios for the DeleteRecords, RebuildPartition, and DropPartition methods. However, it would be beneficial to add more descriptive test case names to improve readability and make it easier to understand what each test case is validating.

Code feedback

pkg/mau/storage/mau_test.go

• Line 79: It would be helpful to add a comment before each test case explaining what it is testing. This will improve the readability and maintainability of the tests. [medium]

pkg/mau/storage/mock/mau.go

• Line 33: The comment Source: mau.go is not needed and can be removed. [medium]
• Line 35: The comment Package mock is a generated GoMock package. is not needed and can be removed. [medium]
• Line 67: The comment // DeleteRecords mocks base method. is not needed and can be removed. [medium]
• Line 76: The comment // DeleteRecords indicates an expected call of DeleteRecords. is not needed and can be removed. [medium]
• Line 87: The comment // DropPartition mocks base method. is not needed and can be removed. [medium]
• Line 96: The comment // DropPartition indicates an expected call of DropPartition. is not needed and can be removed. [medium]
• Line 107: The comment // RebuildPartition mocks base method. is not needed and can be removed. [medium]
• Line 116: The comment // RebuildPartition indicates an expected call of RebuildPartition. is not needed and can be removed. [medium]

Security concerns:

No

[github-actions]

PR Analysis

Main theme

type: Refactoring
description: Addition of a new BatchJob constant and its corresponding value.

PR summary

type: Refactoring
description: This PR adds a new constant BatchJob_MauPartitionDeleter to the BatchJob enum in the service.pb.go file.

Type of PR

Refactoring

PR Feedback:

General suggestions

Move the addition of the new constant to a separate commit to keep the changes focused. This will make it easier to review and understand the purpose of each commit.

Code feedback

relevant file: proto/batch/service.pb.go

suggestion: Separate the addition of the new constant into a separate commit.

relevant line: +BatchJob_MauPartitionDeleter BatchJob = 11

Security concerns:

no

[github-actions]

PR Analysis

Main theme

type: Refactoring
description: Adding a new enum value and updating the corresponding files.

PR summary

type: Refactoring
description: This PR adds a new enum value "MauPartitionDeleter" to the BatchJob enum in the service.proto file. It also updates the proto.lock file, the service_pb.d.ts file, and the service_pb.js file to reflect this change.

Type of PR

Refactoring

PR Feedback:

General suggestions

No general suggestions.

Code feedback

File: a/proto/batch/service.proto

Suggestion 1 (important)

The new enum value "MauPartitionDeleter" is missing a numeric value associated with it. It would be helpful to assign a unique numeric value to ensure compatibility with other systems that may rely on the enum values. For example, you could assign the value 11 to "MauPartitionDeleter".

+  MauPartitionDeleter = 11;

File: a/proto/proto.lock

Suggestion 1 (medium)

The "optional" field is missing in the "ProgressiveRolloutManualScheduleClause", "ProgressiveRolloutTemplateScheduleClause", "ProgressiveRollout.Status", and "ProgressiveRollout.Type" fields. Adding the optional field with a value of true would allow these fields to be nullable in the generated code.

-                "type": "ProgressiveRolloutManualScheduleClause"
+                "type": "ProgressiveRolloutManualScheduleClause",
+                "optional": true

-                "type": "ProgressiveRolloutTemplateScheduleClause"
+                "type": "ProgressiveRolloutTemplateScheduleClause",
+                "optional": true

-                "type": "ProgressiveRollout.Status"
+                "type": "ProgressiveRollout.Status",
+                "optional": true

-                "type": "ProgressiveRollout.Type"
+                "type": "ProgressiveRollout.Type",
+                "optional": true

File: a/ui/web-v2/apps/admin/src/proto/batch/service_pb.d.ts

Suggestion 1 (medium)

The BatchJobMap interface is missing a property for the new enum value "MAUPARTITIONDELETER". Adding the following property to the interface would allow the enum value to be used in the generated code.

+  MAUPARTITIONDELETER: 11;

File: a/ui/web-v2/apps/admin/src/proto/batch/service_pb.js

Suggestion 1 (medium)

The BatchJob object is missing a property for the new enum value "MAUPARTITIONDELETER". Adding the following property to the object would allow the enum value to be used in the generated code.

+  MAUPARTITIONDELETER: 11

Security concerns:

no

[cre8ivejp]

Nice work!
Thank you!