Add partition delete logic in `sync` #243

sudiptob2 · 2024-09-27T17:02:43Z

closes #239

dave-gantenbein · 2024-09-30T14:18:05Z

internal/database/repository/partitions.go

@@ -54,19 +57,18 @@ func (s *PostgresRepository) UpsertPartitions(ctx context.Context, partitions []
 	return nil
 }

-func (s *PostgresRepository) GetAllPartitions(ctx context.Context) ([]*dao.PartitionInfo, error) {
+func (s *PostgresRepository) GetAllPartitions(ctx context.Context) ([]*model.PartitionInfo, error) {


Should we be preparing for pagination parameters here? It seems unlikely we'd ever want to return everything to the front end all at once...

Thanks, we also have several other APIs that need to be updated with pagination. This task is tracked in #245

nikola-jokic · 2024-10-01T11:24:51Z

internal/yunikorn/sync.go

+			return fmt.Errorf("could not upsert partitions: %w", err)
+		}
+		// Delete partitions that are not present in the API response
+		deleteCandidates, err := s.findPartitionDeleteCandidates(ctx, partitions)


since workqueue has retries, you should probably build up deleteCandidates before pushing the task on it. If we ever retry, there is no need to re-calculate them

nikola-jokic · 2024-10-01T11:31:36Z

internal/database/repository/partitions.go

+	}
+
+	deletedAt := time.Now().Unix()
+	query := `UPDATE partitions SET deleted_at = $1 WHERE name = ANY($2)`


You should probably target these partitions by ID. Imagine that we had a partition named "example". Then we removed it. Then we re-installed it. Then we remove it again. Currently, in the database, we have multiple partition records with the name "example", but only one of them should have deleted_at set to now.

nikola-jokic · 2024-10-01T11:32:40Z

internal/database/repository/partitions.go

@@ -54,19 +57,18 @@ func (s *PostgresRepository) UpsertPartitions(ctx context.Context, partitions []
 	return nil
 }

-func (s *PostgresRepository) GetAllPartitions(ctx context.Context) ([]*dao.PartitionInfo, error) {
+func (s *PostgresRepository) GetAllPartitions(ctx context.Context) ([]*model.PartitionInfo, error) {


Here, you should probably get only active partitions. We don't need to get deleted partitions to operate on them.
We should also change the name, so we know that we are fetching active partitions.

Hi, Thanks for the awesome reviews 🚀
I found that, GetAllPartitions is also being used by the webservice here. I think frontend might need the deleted partition data as well to show some graphical design on them. Therefore, I updated the logic in findPartitionDeleteCandidates()

nikola-jokic · 2024-10-01T13:51:39Z

internal/yunikorn/sync.go

+	}
+
+	// Fetch partitions from the database
+	dbPartitions, err := s.repo.GetAllPartitions(ctx)


Since we are skipping stuff that is deleted, why not simply fetch partitions from the database where deleted_at is null
This is especially important when the amount of data builds up. ☺️

Yeah true, I found that, GetAllPartitions is also being used by the web service here. Should we embed the is null logic in GetAllPartitions() as this method is not used in internal call only.

I think we have the following two approaches here,

Implement a different GetAllActivePartition() for internal call.

once we add support for pagination and filtering we will be able to filter the null only partitions in our internal calls when needed.

Let me know your thoughts

Implemented seperate GetActivePartitions()

nikola-jokic

We can push the work of finding partitions to the database to do something like:

DELETE FROM partitions where id IN (
  select id from partitions where deleted_at IS NULL AND NOT(id = ANY($1))
)

But I will leave it up to you if you want to do it now or we can proceed with this for now

sudiptob2 · 2024-10-03T13:44:34Z

We can push the work of finding partitions to the database to do something like:
Hmm.. seems cleaner, we can reduce code in this way 👍

nikola-jokic · 2024-10-04T11:35:36Z

internal/database/repository/partitions.go

@@ -88,3 +126,25 @@ func (s *PostgresRepository) GetAllPartitions(ctx context.Context) ([]*dao.Parti
 	}
 	return partitions, nil
 }
+
+func (s *PostgresRepository) DeletePartitions(ctx context.Context, partitions []*dao.PartitionInfo) error {


Rename to DeleteActivePartitionsNotInPartitions or something similar. The name is misleading ☺️

Updated the name, but still not sure about the wording 😐. Let me know how it sounds.

sudiptob2 force-pushed the feat/239/handle-partition-delete branch from bda4b34 to 05b7746 Compare September 30, 2024 13:03

sudiptob2 requested a review from nikola-jokic September 30, 2024 13:25

sudiptob2 marked this pull request as ready for review September 30, 2024 13:25

dave-gantenbein reviewed Sep 30, 2024

View reviewed changes

sudiptob2 force-pushed the feat/239/handle-partition-delete branch from 444946f to cadcc6e Compare September 30, 2024 17:32

nikola-jokic requested changes Oct 1, 2024

View reviewed changes

sudiptob2 requested a review from nikola-jokic October 1, 2024 13:35

nikola-jokic requested changes Oct 1, 2024

View reviewed changes

sudiptob2 requested a review from nikola-jokic October 1, 2024 15:59

sudiptob2 force-pushed the feat/239/handle-partition-delete branch from 307e3df to 639f299 Compare October 2, 2024 15:25

nikola-jokic previously approved these changes Oct 3, 2024

View reviewed changes

sudiptob2 dismissed nikola-jokic’s stale review via efc8d8f October 3, 2024 14:52

sudiptob2 force-pushed the feat/239/handle-partition-delete branch 2 times, most recently from efc8d8f to 4f06b06 Compare October 3, 2024 15:31

sudiptob2 requested review from nikola-jokic and dave-gantenbein October 3, 2024 15:37

nikola-jokic requested changes Oct 4, 2024

View reviewed changes

sudiptob2 requested a review from nikola-jokic October 4, 2024 13:23

sudiptob2 added 11 commits October 8, 2024 10:48

add deleted_ad filed in the partition table.

5f6f82e

implement deletePartition() repo method.

887119a

Add delete logic in partition sync

af756ab

lint

b94fb60

lint

7f884ac

add unit test for findPartitionDeleteCandidates

6a2ec48

update delete logic

0d02068

update delete logic

c9f32eb

update test

fe66066

modify migration directly

50057e1

push finding delete candidate logic into db layer.

6aa1e25

rename deletePartition repo method

181b855

sudiptob2 force-pushed the feat/239/handle-partition-delete branch from de4dcb8 to 181b855 Compare October 8, 2024 14:48

nikola-jokic approved these changes Oct 11, 2024

View reviewed changes

Merge branch 'main' into feat/239/handle-partition-delete

6100059

sudiptob2 merged commit f5a2063 into G-Research:main Oct 11, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add partition delete logic in `sync` #243

Add partition delete logic in `sync` #243

sudiptob2 commented Sep 27, 2024

dave-gantenbein Sep 30, 2024

sudiptob2 Sep 30, 2024

nikola-jokic Oct 1, 2024

nikola-jokic Oct 1, 2024

nikola-jokic Oct 1, 2024

sudiptob2 Oct 1, 2024

nikola-jokic Oct 1, 2024

sudiptob2 Oct 1, 2024

sudiptob2 Oct 1, 2024

nikola-jokic left a comment •

edited

Loading

sudiptob2 commented Oct 3, 2024

nikola-jokic Oct 4, 2024

sudiptob2 Oct 4, 2024

Add partition delete logic in sync #243

Add partition delete logic in sync #243

Conversation

sudiptob2 commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikola-jokic left a comment • edited Loading

Choose a reason for hiding this comment

sudiptob2 commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add partition delete logic in `sync` #243

Add partition delete logic in `sync` #243

nikola-jokic left a comment •

edited

Loading