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

endpoint to return backup queue, metatdata and pending files count #1100

Open
wants to merge 2 commits into
base: 4.x
Choose a base branch
from
Open

endpoint to return backup queue, metatdata and pending files count #1100

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10840be
endpoint to return backup queue, metatdata and pending files count
Jun 5, 2024
cceb677
Updated backup DirectorySize interface to get the file count for snap…
Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions priam/src/main/java/com/netflix/priam/backup/IncrementalBackup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@
import com.netflix.priam.scheduler.SimpleTimer;
import com.netflix.priam.scheduler.TaskTimer;
import java.io.File;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Set;
import java.util.stream.Stream;
import javax.inject.Inject;
import javax.inject.Singleton;
import org.apache.commons.io.FileUtils;
Expand Down Expand Up @@ -138,4 +140,19 @@ private Void deleteIfEmpty(File dir) {
if (FileUtils.sizeOfDirectory(dir) == 0) FileUtils.deleteQuietly(dir);
return null;
}

public static int countFilesInBackupDir(IConfiguration config) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is that we not co-mingle this with classes dedicated to uploading incremental backups or taking snapshots. Please put these methods in backupV2Servlet or in a class with the purpose of counting the number of files. You'll need to share the constants INCREMENTAL_BACKUP_FOLDER and SNAPSHOT_FOLDER between all of them. The best place would be as static constants in BackupRestoreUtil.

Now that I think about it, the total bytes is really what is of interest. There is already an interface called DirectorySize and an implementation for Snapshot directories called SnapshotDirectorySize. You could implement a similar one for incrementals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed this in new commit

int totalFileCount = 0;
Set<Path> backupDirectories =
AbstractBackup.getBackupDirectories(config, INCREMENTAL_BACKUP_FOLDER);
for (Path backupDir : backupDirectories) {
try (Stream<Path> stream = Files.list(backupDir)) {
totalFileCount += stream.filter(Files::isRegularFile).count();
} catch (Exception e) {
logger.error("Failed to get files in backups directory. {}", e.getMessage());
e.printStackTrace();
}
}
return totalFileCount;
}
}
9 changes: 9 additions & 0 deletions priam/src/main/java/com/netflix/priam/backupv2/BackupV2Service.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import com.netflix.priam.scheduler.PriamScheduler;
import com.netflix.priam.scheduler.TaskTimer;
import com.netflix.priam.tuner.CassandraTunerService;
import java.util.HashMap;
import java.util.Map;
import javax.inject.Inject;
import org.apache.commons.lang3.math.Fraction;

Expand Down Expand Up @@ -101,4 +103,11 @@ public void updateServicePre() throws Exception {

@Override
public void updateServicePost() throws Exception {}

public Map<String, Integer> countPendingBackupFiles() throws Exception {
Map<String, Integer> backupFiles = new HashMap<String, Integer>();
backupFiles.put("snapshotFiles", snapshotMetaTask.countFilesInSnapshotDir(configuration));
backupFiles.put("incrementalFiles", IncrementalBackup.countFilesInBackupDir(configuration));
return backupFiles;
}
}
23 changes: 23 additions & 0 deletions priam/src/main/java/com/netflix/priam/backupv2/SnapshotMetaTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import java.util.concurrent.Executors;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.stream.Stream;
import javax.inject.Inject;
import javax.inject.Named;
import javax.inject.Singleton;
Expand Down Expand Up @@ -442,4 +443,26 @@ public void onFailure(Throwable t) {
};
Futures.addCallback(future, callback, MoreExecutors.directExecutor());
}

public int countFilesInSnapshotDir(IConfiguration config) throws Exception {
int totalFileCount = 0;
Set<Path> snapshotDirectories =
AbstractBackup.getBackupDirectories(config, SNAPSHOT_FOLDER);
for (Path snapshotDir : snapshotDirectories) {
try (DirectoryStream<Path> directoryStream =
Files.newDirectoryStream(snapshotDir, Files::isDirectory)) {
for (Path backupDir : directoryStream) {
if (backupDir.toFile().getName().startsWith(SNAPSHOT_PREFIX)) {
try (Stream<Path> stream = Files.list(backupDir)) {
totalFileCount += stream.filter(Files::isRegularFile).count();
}
}
}
} catch (Exception e) {
logger.error("Failed to get files in snapshot directory. {}", e.getMessage());
e.printStackTrace();
}
}
return totalFileCount;
}
}
36 changes: 35 additions & 1 deletion priam/src/main/java/com/netflix/priam/resources/BackupServletV2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,24 @@

package com.netflix.priam.resources;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.priam.PriamServer;
import com.netflix.priam.backup.*;
import com.netflix.priam.backupv2.BackupTTLTask;
import com.netflix.priam.backupv2.BackupV2Service;
import com.netflix.priam.backupv2.IMetaProxy;
import com.netflix.priam.backupv2.SnapshotMetaTask;
import com.netflix.priam.config.IConfiguration;
import com.netflix.priam.merics.BackupMetrics;
import com.netflix.priam.notification.BackupNotificationMgr;
import com.netflix.priam.utils.DateUtil;
import com.netflix.priam.utils.DateUtil.DateRange;
import com.netflix.priam.utils.GsonJsonSerializer;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import javax.inject.Inject;
Expand All @@ -55,6 +60,9 @@ public class BackupServletV2 {
private final Provider<AbstractBackupPath> pathProvider;
private final BackupV2Service backupService;
private final BackupNotificationMgr backupNotificationMgr;
private final PriamServer priamServer;

private final BackupMetrics backupMetrics;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused. Please remove it.

private static final String REST_SUCCESS = "[\"ok\"]";

@Inject
Expand All @@ -68,7 +76,9 @@ public BackupServletV2(
@Named("v2") IMetaProxy metaV2Proxy,
Provider<AbstractBackupPath> pathProvider,
BackupV2Service backupService,
BackupNotificationMgr backupNotificationMgr) {
BackupNotificationMgr backupNotificationMgr,
PriamServer priamServer,
BackupMetrics backupMetrics) {
this.backupStatusMgr = backupStatusMgr;
this.backupVerification = backupVerification;
this.snapshotMetaService = snapshotMetaService;
Expand All @@ -78,6 +88,8 @@ public BackupServletV2(
this.pathProvider = pathProvider;
this.backupService = backupService;
this.backupNotificationMgr = backupNotificationMgr;
this.priamServer = priamServer;
this.backupMetrics = backupMetrics;
}

@GET
Expand Down Expand Up @@ -181,4 +193,26 @@ public Response list(@PathParam("daterange") String daterange) throws Exception
.collect(Collectors.toList())))
.build();
}

@GET
@Path("/state/{hours}")
public Response backupState(@PathParam("hours") int hours) throws Exception {
Map<String, Object> responseMap = new HashMap<>();

responseMap.put("tasksQueued", fs.getUploadTasksQueued());
responseMap.put("queueSize", priamServer.getConfiguration().getBackupQueueSize());
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not require information that only the local node has. Please read it from fast properties. Or better yet, don't include it in the calculations as we might move to a model where the queue is small and populated as needed and the only metric that matters is the remaining files on disk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This FP is not set for every cluster and we need it to compute the file threshold for which we want to wait for backup upload to complete.

Copy link

Choose a reason for hiding this comment

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

Since we are already making a call to Priam what is the harm in checking this here? The downside I see to checking FPs is we now rely on yet another system and per Ayushis comment would have the default hardcoded in multiple places.

for (Map.Entry<String, Integer> entry :
backupService.countPendingBackupFiles().entrySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to make a distinction between snapshot and incremental backup files in this case? Prefer to combine them into a total count if we can afford to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed this in new commit

responseMap.put(entry.getKey(), entry.getValue());
}

List<BackupMetadata> latestBackupMetadata =
Copy link
Contributor

Choose a reason for hiding this comment

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

We can get this metadata from elsewhere. Please don't make the local node do it. Prefer creating an Antigravity endpoint that fetches this information from cass_cde.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want a node specific backup metadata in the realtime to compute if the backup was started in last 30 mins.

Copy link

Choose a reason for hiding this comment

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

i would also be concerned that we are depending on an additional system as I commented above. Is there a harm in checking this here?

backupStatusMgr.getLatestBackupMetadata(
new DateRange(Instant.now().minus(hours, ChronoUnit.HOURS), Instant.now()));
responseMap.put("latestBackupMetadata", latestBackupMetadata);

ObjectMapper mapper = new ObjectMapper();
String jsonResponse = mapper.writeValueAsString(responseMap);
return Response.ok(jsonResponse).build();
}
}
Loading