Implement remove command #89

rk1274 · 2025-02-12T15:37:34Z

https://jira.sanger.ac.uk/browse/HSI-64

server/server.go

server/setdb.go

cmd/remove.go

server/server.go

server/setdb.go

set/db.go

y-popov · 2025-02-13T16:11:07Z

We also need to store remove requests in the database, so if a server goes down we will be able to recover interrrupted removal process

put/baton.go

put/put.go

server/server.go

server/server_test.go

set/db.go

…t set

… to status

…emoved from DB and delint

- Rename functions in discoveryCoordinator - Move buildRemoveItemDef to set pkg - Make mock mutex private

baton/baton.go

internal/mock.go

mjkw31

LGTM.

server/server_test.go

…folder

…ncomplete sets

remove/remove.go

server/setdb.go

requested changes

remove/remove.go

server/setdb.go

sb10 · 2025-04-02T08:15:07Z

internal/error.go

These new types should not be internal; external code won't be able to type check against these. The structs should be defined in the pkg where they are used so that callers can ask if the error is specifically a kind of error from that pkg.

Maybe PathError can be here if it's used in multiple pkgs and callers don't need to do their own type check, but not the more specific Dir one.

So we create a DirNotEmptyError using NewDirNotEmpty in baton and mock, and type check against it in remove. So if we put that error in the remove, baton and mock would need to import remove that makes no sense and will cause import cycle because remove already imports internal.
We guess the best place for that error is in internal

Even if the error stays in the internal, external users still will be able to use the error using ibackup.internal.DirNotEmptyError, won't they?

Michael explained that internal is a special folder that is not available to external users.
How about making errs package with all the ibackup errors?

The error type should be defined in the package that creates that error, not in the package that is doing the type check. So it should be defined in baton, and hopefully mock can import from baton to use it?

Actually, it shouldn't be defined in an interface implementation package. We should sit together and look at the code...

rk1274 requested review from sb10 and mjkw31 February 12, 2025 15:37

mjkw31 requested changes Feb 12, 2025

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

server/setdb.go Outdated Show resolved Hide resolved

server/setdb.go Outdated Show resolved Hide resolved

sb10 requested changes Feb 13, 2025

View reviewed changes

cmd/remove.go Outdated Show resolved Hide resolved

server/server.go Outdated Show resolved Hide resolved

server/setdb.go Outdated Show resolved Hide resolved

set/db.go Outdated Show resolved Hide resolved

rk1274 requested a review from sb10 February 17, 2025 13:26

sb10 requested changes Feb 17, 2025

View reviewed changes

rk1274 and others added 23 commits February 18, 2025 11:51

Add implentation to remove files and dirs from a set

200ae79

Add implementation to remove metadata and files from iRODS

2d73f4d

Add test for removing hardlink files

ec3ce10

Add implementation for removing hardlinks and dirs from iRODS

45ab258

Add validation to check if files/dirs are in the set

f3c6283

Refactor removeFilesFromIRODS and removeDirsFromIRODS functions

b04ee4e

Fix wrong implementation about deleting hardlink inodes file

e8bfdf2

Add implementation to remove requester from iRODS metadata

b879294

Add extra validation for different user`s sets with the same name WIP

fc55102

Refactor handleSetsAndRequesters

3e418f2

Fix bug when removing a failed file that was successful in a differen…

043d554

…t set

Update set counts after removal WIP

26fc4be

Refactor SetNewCounts to recalculate file counts

ace746b

Combine removeFiles and removeDirs endpoints and add recently removed…

73708fe

… to status

Implement removeQueue WIP

48dc79d

Finish implentation for removeQueue WIP

25131d3

Finish removeQueue implementation

973a34d

Update status to have a Removal status line when removing

53aa3bb

Handle removal retries

cd1bae3

Refactor removeQueue implementation

18c0cb8

Add handling for when an item is removed from iRODS but fails to be r…

812e3a0

…emoved from DB and delint

Delint and fix tests

6818231

Refactor classification of a path to use db instead of file system

2b954c6

PR fixes

741c472

- Rename functions in discoveryCoordinator - Move buildRemoveItemDef to set pkg - Make mock mutex private

rk1274 requested a review from sb10 March 18, 2025 08:59

mjkw31 requested changes Mar 18, 2025

View reviewed changes

baton/baton.go Outdated Show resolved Hide resolved

baton/baton.go Outdated Show resolved Hide resolved

internal/mock.go Outdated Show resolved Hide resolved

Add further tests for discovery coordinator; fix PR comments

9647a51

mjkw31 previously approved these changes Mar 18, 2025

View reviewed changes

sb10 requested changes Mar 18, 2025

View reviewed changes

server/server_test.go Outdated Show resolved Hide resolved

server/server_test.go Outdated Show resolved Hide resolved

Fix race condition

5032098

rk1274 dismissed mjkw31’s stale review via 5032098 March 19, 2025 13:33

rk1274 added 12 commits March 19, 2025 15:45

Add logging and ignore unexpected errors when removing dirs

4b37676

Reset removal values when readding a set

606e376

Add check for if a sub bucket exists before trying to remove it

7af56ca

Add check in removeEntry for if the bucket exists before using it

9f43aa9

remove discovered folders from the database during removal of a base …

9a337fb

…folder

Don't count duplicate files when removing

01bc5a3

Handle removals for files that failed to upload

863c9ab

Do not allow removals of pending files

ae084e9

Remove from failed bucket during removal

e626234

Make removal work on sets created with old code

eb33804

Finalise issues with sets made with old code and finish issues with i…

a785d17

…ncomplete sets

Delint

a3289b7

rk1274 requested a review from sb10 March 28, 2025 15:56

sb10 previously approved these changes Mar 31, 2025

View reviewed changes

remove/remove.go Outdated Show resolved Hide resolved

server/setdb.go Outdated Show resolved Hide resolved

mjkw31 requested changes Apr 1, 2025

View reviewed changes

remove/remove.go Outdated Show resolved Hide resolved

server/setdb.go Show resolved Hide resolved

rk1274 added 3 commits April 1, 2025 13:57

Add DirRemovalError struct

8c81f7e

Fix UpdateRemoveRequest for directories

cd136e2

Add DirNotEmptyError struct

e0077c0

rk1274 force-pushed the remove-command branch from 4c2bd10 to e0077c0 Compare April 1, 2025 15:24

sb10 requested changes Apr 2, 2025

View reviewed changes

Reset sizeRemoved when readding a set

2d5376f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement remove command #89

Implement remove command #89

rk1274 commented Feb 12, 2025

y-popov commented Feb 13, 2025

mjkw31 left a comment

sb10 Apr 2, 2025

y-popov Apr 2, 2025

y-popov Apr 2, 2025

y-popov Apr 2, 2025 •

edited

Loading

sb10 Apr 3, 2025

sb10 Apr 3, 2025

Implement remove command #89

Are you sure you want to change the base?

Implement remove command #89

Conversation

rk1274 commented Feb 12, 2025

y-popov commented Feb 13, 2025

mjkw31 left a comment

Choose a reason for hiding this comment

sb10 Apr 2, 2025

Choose a reason for hiding this comment

y-popov Apr 2, 2025

Choose a reason for hiding this comment

y-popov Apr 2, 2025

Choose a reason for hiding this comment

y-popov Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

sb10 Apr 3, 2025

Choose a reason for hiding this comment

sb10 Apr 3, 2025

Choose a reason for hiding this comment

y-popov Apr 2, 2025 •

edited

Loading