endpoint to return backup queue, metatdata and pending files count #1100
Conversation
| responseMap.put("tasksQueued", fs.getUploadTasksQueued()); | ||
| responseMap.put("queueSize", priamServer.getConfiguration().getBackupQueueSize()); | ||
| for (Map.Entry<String, Integer> entry : | ||
| backupService.countPendingBackupFiles().entrySet()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Addressed this in new commit
| responseMap.put(entry.getKey(), entry.getValue()); | ||
| } | ||
|
|
||
| List<BackupMetadata> latestBackupMetadata = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We want a node specific backup metadata in the realtime to compute if the backup was started in last 30 mins.
There was a problem hiding this comment.
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?
| Map<String, Object> responseMap = new HashMap<>(); | ||
|
|
||
| responseMap.put("tasksQueued", fs.getUploadTasksQueued()); | ||
| responseMap.put("queueSize", priamServer.getConfiguration().getBackupQueueSize()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
This is unused. Please remove it.
| @@ -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 { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Addressed this in new commit
ayushisingh29
force-pushed
the
feature/backup_state
branch
from
bde4768 to
cceb677
Compare
There was a problem hiding this comment.
Left a 👍 on Matt's comments I agreed with. And a few other thoughts on the remaining comments. I think lets address the ones we agree on and finalize a decision around the outstanding discussions about where to get some of the more static data. Then we can get this merged.
No description provided.