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

sb10 · 2025-02-17T14:01:08Z

put/baton.go

@@ -85,6 +86,34 @@ func GetBatonHandler() (*Baton, error) {
 	return &Baton{}, err
 }

+// GetBatonHandlerWithMetaClient returns a Handler that uses Baton to interact
+// with iRODS and contains a meta client for interacting with metadata. If you
+// don't have baton-do in your PATH, you'll get an error.


There should be some docs and, probably for GetBatonHandler() as well, and probably for server.Config.StorageHandler that makes it explicit you need to call this and not GetBatonHandler() for remove functionality to work.

Is there a better way of arranging things so there aren't 2 GetBaton...() methods? Or some other way of avoiding possible mistakes like this?

sb10 · 2025-02-17T14:02:39Z

put/baton.go

+	it := remotePathToRodsItem(path)
+
+	err := timeoutOp(func() error {
+		_, errl := b.metaClient.RemObj(ex.Args{}, *it)


Write a test that fails due to panic when you use a Baton from GetHandler(), then fix this in some way to return a nicer error. Likewise for queryMeta, RemoveDir and GetMeta.

put/baton.go

put/put.go

sb10 · 2025-02-17T14:09:01Z

put/put.go

@@ -23,7 +23,7 @@
 * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 ******************************************************************************/

-// package put is used to put files in iRODS.
+// package put is used to interact with iRODS.


One possible solution to the confusion over baton GetHandler vs GetBatonHandlerWithMetaClient is that put package remains for putting only, and another package is created with its own interface that embeds put's Handler and has the extra non-put stuff, and it's that interface that the server requires.

@sb10 we implemented your idea in the most recent commit (7d3b...).
Could you take a look and tell whether this looks good or not? In terms of whether it indeed makes sense to split put and remove functions between different packages.

I think in principle it's a valuable change to make, if it can be done right, and if there are callers that need the split.

The benefit of a split in the Handler interface would be if there is any code outside the put package that needs a Handler with just put functions, but not remove functions. Or vice versa.
But it seems like server needs the remove functions, and at least one put function?
Does cmd/put in non-server mode also only want put functions?

If there's a split desired functionality, then yes, we should have 2 different interfaces, and keep the put package as a pure package for putting.

But baton and mock (probably changing to internal/mock) should be their own packages that provide something that implements both interfaces. Getting back to what triggered this, can there be, in some new baton pkg, a New() function that would happily satisfy both put.Handler and remove.Handler? (ie. is there harm in always having the meta bit? Or can the meta bit be auto-vivified when needed?)

The remove interface in the remove package should be called Handler and should only include the methods that it needs to implement its functionality (so probably not a blanket embed of put.Handler).

server/server.go

sb10 · 2025-02-17T14:49:21Z

server/server_test.go

Not sure if the tests are already in here or elsewhere, but there should be tests of the server's new removal functionality that use the mock instead of real iRODS via baton.
There should be tests that explore the behaviour of the user asking for a local path directory to be removed, that should work when local path no longer exists: all files in the directory should still get removed from iRODS (well, the mock).

set/db.go

…t set

… to status

…emoved from DB and delint

…n the server

put/remove.go

sb10

I think you're still going through my previous review, but here are some more things I noticed.

sb10 · 2025-02-18T12:40:02Z

put/baton.go

+	return err
+}
+
+func (b *Baton) QueryMeta(dirToSearch string, meta map[string]string) ([]string, error) {


sb10 · 2025-02-18T12:40:47Z

put/mock.go

+	return meta, nil
+}
+
+func (l *LocalHandler) RemoveDir(path string) error {


The now public methods here need docs.

sb10 · 2025-02-18T12:45:00Z

set/db.go

@@ -243,6 +248,17 @@ func updateDatabaseSetWithUserSetDetails(dbSet, userSet *Set) error {
 	return nil
 }

+func (d *DB) UploadEntry(sid, key string, entry *Entry) error {


sb10 · 2025-02-18T12:47:35Z

server/setdb.go

+	return err
+}
+
+type RemoveReq struct {


… WIP

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

rk1274 added 6 commits February 18, 2025 11:51

Modify code for pull request

621627a

Small PR fixes

4b87f5b

Change implementation to carry out iRODS commands through a handler o…

596919f

…n the server

Store removeReq in db incase server shutdown WIP

540463d

Allow removal to recover if server goes down

345cfbb

Small PR fixes, and optimisation of GetFilesInDir

9ae8992

rk1274 force-pushed the remove-command branch from 0befb81 to 9ae8992 Compare February 18, 2025 11:54

mjkw31 requested changes Feb 18, 2025

View reviewed changes

put/remove.go Outdated Show resolved Hide resolved

sb10 requested changes Feb 18, 2025

View reviewed changes

rk1274 added 4 commits February 18, 2025 15:04

Move removal functionality from put pkg to remove pkg

7d3b389

Add new baton pkg and make remove.handler use baton.GetBatonHandler()…

a4224f3

… WIP

Make remove.handler use baton.GetBatonHandler()

891de16

Refactor code to move baton tests into baton pkg and add new tests

12d625d

rk1274 force-pushed the remove-command branch from 842e8bd to 12d625d Compare February 20, 2025 16:20

Add tests for mock (localHandler)

22ac24f

rk1274 force-pushed the remove-command branch 2 times, most recently from 0193666 to 85750a7 Compare February 21, 2025 15:56

Add tests for remove pkg and refactor remove functions

78ed27e

rk1274 force-pushed the remove-command branch from 85750a7 to 78ed27e Compare February 21, 2025 16:05

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

sb10 Feb 17, 2025

sb10 Feb 17, 2025

sb10 Feb 17, 2025

y-popov Feb 18, 2025 •

edited

Loading

sb10 Feb 18, 2025

sb10 Feb 17, 2025

sb10 left a comment

sb10 Feb 18, 2025

y-popov Feb 21, 2025

sb10 Feb 18, 2025

y-popov Feb 21, 2025

sb10 Feb 18, 2025

y-popov Feb 21, 2025

sb10 Feb 18, 2025

y-popov Feb 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-popov Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sb10 left a 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

Choose a reason for hiding this comment

y-popov Feb 18, 2025 •

edited

Loading