Clean up stale entries from upgradeSegments table by AmatyaAvadhanula · Pull Request #15637 · apache/druid

AmatyaAvadhanula · 2024-01-08T11:03:49Z

The upgradeSegments table contains entries of pending segments allocated by tasks with APPEND locks when there was a REPLACE lock held over the interval.

This PR aims to clean the entries from the metadata store after the task with the REPLACE lock completes by invoking a clean up method before unlocking its locks.
Since this is performed on the TaskLockbox directly, rolling upgrades wouldn't be affected and the clean up would happen automatically once the Overlord is upgraded. Also, the task lockbox is aware of the locks held by the task and this can help us avoid unnecessary calls to the metadata store to clean stale entries.

This PR has:

kfaraz

Using a task action to perform metadata cleanup makes perfect sense. I have left some feedback that should be addressed before this is merged.

There a couple more concerns:

Rolling Upgrade: A task running on an upgraded version would fire the new action but an old Overlord would not be able to recognize it.
Should the new action be fired only for supervisor type tasks? Atleast in the impl of the task action, we should do some filtering and proceed to DELETE entries only for relevant supervisor tasks with REPLACE context. Otherwise, we would just be firing redundant DELETE statements on the metadata.

server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java

kfaraz · 2024-01-09T03:59:19Z

server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java

   * @return Map from append Segment ID to REPLACE lock version
   */
-  private Map<String, String> getAppendSegmentsCommittedDuringTask(
+  @VisibleForTesting


Please don't do this. Try to find some other cleaner way to test this.

kfaraz · 2024-01-09T04:02:06Z

...xing-service/src/main/java/org/apache/druid/indexing/common/actions/CleanMetadataAction.java

+import com.fasterxml.jackson.core.type.TypeReference;
+import org.apache.druid.indexing.common.task.Task;
+
+public class CleanMetadataAction implements TaskAction<Void>


Please add a javadoc here.
I would prefer using some kind of response object rather than void. This response object could potentially contain the information of whether the cleanup was successful or not, how many rows of which table were deleted, etc. (probably not needed in this PR, but let's return something meaningful anyway.)

Also, rename:

Suggested change

public class CleanMetadataAction implements TaskAction<Void>

public class CleanupMetadataAction implements TaskAction<Void>

kfaraz · 2024-01-09T04:04:51Z

indexing-service/src/test/java/org/apache/druid/indexing/common/task/AbstractTaskTest.java


-    // call it 3 times, once to update location in setup, then one for status and location in cleanup
-    Mockito.verify(taskActionClient, times(3)).submit(any());
+    // call it 3 times, once to update location in setup, then one for status and location in cleanup,


Suggested change

// call it 3 times, once to update location in setup, then one for status and location in cleanup,

// call it 4 times, once to update location in setup, then one for status and location in cleanup,

AmatyaAvadhanula · 2024-01-09T04:17:32Z

@kfaraz, thank you for the feedback.

Rolling Upgrade: A task running on an upgraded version would fire the new action but an old Overlord would not be able to recognize it.

Apologies, I had missed this.
Do you have a suggestion to make the TaskAction work during a rolling upgrade?

An alternative could be to invoke the cleanup from TaskLockbox directly when the task is removed from the set of active tasks instead of using a new task action.

kfaraz · 2024-01-09T06:06:15Z

An alternative could be to invoke the cleanup from TaskLockbox directly when the task is removed from the set of active tasks instead of using a new task action.

Is there any cleanup which is currently performed by the TaskLockbox?

AmatyaAvadhanula · 2024-01-09T06:07:13Z

Is there any cleanup which is currently performed by the TaskLockbox?

Yes, the TaskLockbox is responsible for clearing the lock entries from the metadata store

The TaskQueue could also call a task action while removing a task from its set of active tasks. This happens on the Overlord itself so there wouldn't be an issue with rolling upgrades.

kfaraz · 2024-01-11T03:59:44Z

@AmatyaAvadhanula , let's keep it simple. Since the new task action is responsible only for cleanup of upgrade segments (something which doesn't happen in the current version anyway), let's just put the submission of the task action in a try catch. If it succeeds, cool. If the OL cannot identify the task action and throws an exception, we just log the exception saying that the OL is probably on an old version. This would ensure that the task itself doesn't fail if the OL is on an old version.

Also, please make sure you test out this scenario.

…ments_table

kfaraz · 2024-01-17T12:57:48Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java

    try {
      try {
        log.info("Removing task[%s] from activeTasks", task.getId());
+        if (findLocksForTask(task).stream().anyMatch(lock -> lock.getType() == TaskLockType.REPLACE)) {


Should we use findReplaceLocksForTask method instead? But it would return only non-revoked locks.

kfaraz

Minor comments, otherwise LGTM

kfaraz · 2024-01-17T12:59:04Z

server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java

  DataSegment retrieveSegmentForId(String id, boolean includeUnused);
+
+  /**
+   * Clean entries in upgrade segments table after the corresponding replace task has ended


Suggested change

* Clean entries in upgrade segments table after the corresponding replace task has ended

* Delete entries from upgrade segments table after the corresponding replace task has ended

AmatyaAvadhanula · 2024-01-17T15:19:44Z

@kfaraz , Thank you for the review!
Merging since failures are unrelated

* Clean up stale entries from upgradeSegments table

Clean up stale entries from upgradeSegments table

9394085

github-actions bot added the Area - Ingestion label Jan 8, 2024

Fix failing test

f047b96

kfaraz self-requested a review January 8, 2024 12:14

kfaraz requested changes Jan 9, 2024

View reviewed changes

AmatyaAvadhanula added 3 commits January 16, 2024 11:41

Merge remote-tracking branch 'upstream/master' into clean_upgrade_seg…

988cb63

…ments_table

Merge remote-tracking branch 'upstream/master' into clean_upgrade_seg…

bbe3a02

…ments_table

Address feedback

b828587

AmatyaAvadhanula marked this pull request as draft January 17, 2024 05:33

AmatyaAvadhanula added 2 commits January 17, 2024 15:28

Clean up while releasing locks on the Overlord

d65a608

clean up

e1e9e72

AmatyaAvadhanula marked this pull request as ready for review January 17, 2024 10:01

AmatyaAvadhanula requested a review from kfaraz January 17, 2024 10:04

kfaraz reviewed Jan 17, 2024

View reviewed changes

kfaraz approved these changes Jan 17, 2024

View reviewed changes

kfaraz added this to the Druid 29.0.0 milestone Jan 17, 2024

Better docs

dbc4d34

AmatyaAvadhanula merged commit a26defd into apache:master Jan 17, 2024

kfaraz deleted the clean_upgrade_segments_table branch January 18, 2024 05:00

AmatyaAvadhanula added a commit to AmatyaAvadhanula/druid that referenced this pull request Jan 29, 2024

Clean up stale entries from upgradeSegments table (apache#15637)

09c0234

* Clean up stale entries from upgradeSegments table

AmatyaAvadhanula mentioned this pull request Jan 29, 2024

[Backport] Clean up stale entries from upgradeSegments table (#15637) #15774

Merged

LakshSingla pushed a commit that referenced this pull request Jan 30, 2024

Clean up stale entries from upgradeSegments table (#15637) (#15774)

ba1b31b

* Clean up stale entries from upgradeSegments table

	public class CleanMetadataAction implements TaskAction<Void>
	public class CleanupMetadataAction implements TaskAction<Void>

	// call it 3 times, once to update location in setup, then one for status and location in cleanup,
	// call it 4 times, once to update location in setup, then one for status and location in cleanup,

	* Clean entries in upgrade segments table after the corresponding replace task has ended
	* Delete entries from upgrade segments table after the corresponding replace task has ended

Conversation

AmatyaAvadhanula commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmatyaAvadhanula commented Jan 9, 2024

Uh oh!

kfaraz commented Jan 9, 2024

Uh oh!

AmatyaAvadhanula commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfaraz commented Jan 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmatyaAvadhanula commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AmatyaAvadhanula commented Jan 8, 2024 •

edited

Loading

AmatyaAvadhanula commented Jan 9, 2024 •

edited

Loading