feat(warehouse): added base support for s3 datalake deletion as part of regulation API. #2515

abhimanyubabbar · 2022-09-30T15:28:22Z

Description

As per the regulation api, we are adding support to remove the user information data from s3 datalake.

Notion Ticket

https://www.notion.so/rudderstacks/S3-Datalake-Data-Deletion-Request-Dev-tasks-dfdba07974da4ffdb4b40af163bfdefc

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

codecov · 2022-10-17T11:33:02Z

Codecov Report

Base: 43.77% // Head: 43.84% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (d5fa7dd) compared to base (0ec74d1).
Patch coverage: 56.92% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2515      +/-   ##
==========================================
+ Coverage   43.77%   43.84%   +0.06%     
==========================================
  Files         186      187       +1     
  Lines       39890    40015     +125     
==========================================
+ Hits        17463    17545      +82     
- Misses      21332    21370      +38     
- Partials     1095     1100       +5

Impacted Files	Coverage Δ
regulation-worker/internal/service/looper.go	`0.00% <0.00%> (ø)`
services/filemanager/filemanager.go	`51.28% <0.00%> (-1.82%)`	⬇️
warehouse/jobs/runner.go	`0.00% <0.00%> (ø)`
regulation-worker/internal/delete/batch/batch.go	`48.28% <56.05%> (+0.50%)`	⬆️
...n-worker/internal/delete/batch/filehandler/gzip.go	`62.68% <62.68%> (ø)`
...orker/internal/delete/batch/filehandler/parquet.go	`65.35% <65.35%> (ø)`
regulation-worker/cmd/main.go	`58.92% <100.00%> (+0.74%)`	⬆️
regulation-worker/internal/client/client.go	`67.39% <100.00%> (-0.47%)`	⬇️
regulation-worker/internal/delete/delete.go	`100.00% <100.00%> (ø)`
regulation-worker/internal/service/service.go	`70.83% <100.00%> (ø)`
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lvrach

minor or issues not introduced in this PR, but I will need a second pass

regulation-worker/internal/client/client.go

regulation-worker/internal/delete/batch/batch.go

lvrach · 2022-10-17T11:33:45Z

regulation-worker/internal/delete/batch/batch.go

-		return fmt.Errorf("error while opening file, %w", err)
-	}
-	defer cleanCompressedFilePtr.Close()
+			f, err = os.OpenFile(filepath.Join(statusTrackerTmpDir, StatusTrackerFileName), os.O_CREATE|os.O_RDWR, 0o644)


Since we removed os.O_TRUNC, if the file already exists, we are going to error, correct?

It is safer this way, I just wanted to make sure this was the intention behind this change.

I haven't changed this implementation.

https://github.com/rudderlabs/rudder-server/blob/master/regulation-worker/internal/delete/batch/batch.go#L473

Probably did a bit more code movement, so it's fuzzing the boundaries.

regulation-worker/internal/delete/batch/batch.go

achettyiitr · 2022-10-19T05:34:03Z

services/filemanager/filemanager.go

@@ -63,6 +63,10 @@ func init() {
 // New returns FileManager backed by configured provider
 func (*FileManagerFactoryT) New(settings *SettingsT) (FileManager, error) {
 	switch settings.Provider {
+	case "S3_DATALAKE":


Why do we need a duplicate fileMangaer for S3 Datalake when we can use S3?

The provider comes in from above and it has S3_DATALAKE in it hardcoded. Let me see what change can I make !

saurav-malani · 2022-10-26T12:32:31Z

regulation-worker/internal/delete/batch/batch.go

 	StatusTrackerFileName = "rudderDeleteTracker.txt"
-	supportedDestinations = []string{"S3"}
+	supportedDestinations = []string{"S3", "S3_DATALAKE"}
 )

 const listMaxItem int64 = 1000


I believe since we have defined FilesLimit in BatchManager struct, we don't need this anymore.

I'll fix it.

saurav-malani · 2022-10-26T12:40:47Z

regulation-worker/internal/client/client.go

-	method := "GET"
-	genEndPoint := "/dataplane/workspaces/{workspace_id}/regulations/workerJobs"
-	url := fmt.Sprint(j.URLPrefix, prepURL(genEndPoint, j.WorkspaceID))
+	url := fmt.Sprintf("%s/dataplane/workspaces/%s/regulations/workerJobs", j.URLPrefix, j.WorkspaceID)


Creating url like this is indeed more readable & better. We can use this same approach to create url in UpdateStatus func also & can get rid of prepURL func.

saurav-malani · 2022-10-26T13:00:04Z

regulation-worker/internal/delete/batch/filehandler/local_file.go

@@ -0,0 +1,21 @@
+package filehandler


I feel file name of this file decreases it's readability. Not sure what would an apt name, how about fileHandler.go .

Also, we might want to change the directory name fileHandler from filehandler too.

Usually names are camelcased in golang but filenames are snakecased in popular repository. The name local_file indicates the interface of local file handling. Let me know if you wanna have an offline discussion.

saurav-malani · 2022-10-26T14:05:19Z

regulation-worker/internal/delete/batch/batch.go

-	defer tmpFilePtr.Close()
-	absPath, err := filepath.Abs(tmpFilePtr.Name())
+
+	defer f.Close()


the reason I am ignoring error here is, it is an overkill to abort job & send failed status to manager even when we failed to close file. I believe we can afford to ignore it.

Suggested change

defer f.Close()

defer func(){_=f.Close()}()

saurav-malani · 2022-10-26T14:08:23Z

regulation-worker/internal/delete/batch/batch.go

+	if err := f.Close(); err != nil {
+		return nil, fmt.Errorf("closing file: %w", err)
 	}


since we are already defering in line 121

Suggested change

if err := f.Close(); err != nil {

return nil, fmt.Errorf("closing file: %w", err)

}

This is somewhat a different way to cleanly handle closing of file. Usually we wanna handle the error that comes with file closing, so we handle it multiple times in such a fashion.

saurav-malani · 2022-10-26T14:15:01Z

regulation-worker/internal/delete/batch/batch.go

-// Note: download happens concurrently in 5 go routine by default.
-func (b *Batch) download(_ context.Context, completeFileName string) (string, error) {
-	pkgLogger.Debugf("downloading file: %v", completeFileName)
+func (_ *Batch) cleanedFiles(_ context.Context, path string, job *model.Job) ([]string, error) {


Given that the list of cleaned files could we really long, won't it make sense to return a pointer instead of []string?

func name suggestion: getCleanedFiles

Go doesn't provide automatic support for getters and setters. There's nothing wrong with providing getters and setters yourself, and it's often appropriate to do so, but it's neither idiomatic nor necessary to put Get into the getter's name. If you have a field called owner (lower case, unexported), the getter method should be called Owner (upper case, exported), not GetOwner. The use of upper-case names for export provides the hook to discriminate the field from the method. A setter function, if needed, will likely be called SetOwner.

I was reading this in effective go a while back where I came across this advice to not place get in the name of the function.

Also why would a pointer be beneficial ? In go I think, array's are passed by value and cost is pretty minimal right ?

saurav-malani · 2022-10-26T14:50:10Z

regulation-worker/internal/delete/batch/batch.go

 			return model.JobStatusFailed
 		}
+
+		pkgLogger.Infof("successfully completed loop of ")


Suggested change

pkgLogger.Infof("successfully completed loop of ")

pkgLogger.Infof("successfully completed loop")

saurav-malani · 2022-10-26T14:56:53Z

regulation-worker/internal/delete/batch/batch.go

 	mu         sync.Mutex
 	FM         filemanager.FileManager


Won't it be better to put file handler inside Batch struct, given it is specific to an instance of deletion job?

Suggested change

mu sync.Mutex

FM filemanager.FileManager

mu sync.Mutex

FM filemanager.FileManager

filehandlers map[string]filehandler.LocalFileHandler

The thing is filehandler needs to be created on processing of every file as they store the intermediate results to allow for cleaner interface. We have added a factory to instantiate a filehandler everytime we see a file of that sorts. So this might not work.

saurav-malani · 2022-10-26T15:04:21Z

regulation-worker/internal/delete/batch/batch.go

-				}
-			}
-			absPath, err := filepath.Abs(statusTrackerFilePtr.Name())
+		fName, err := batch.download(ctx, filepath.Join(prefix, StatusTrackerFileName))


Suggestion to increase code readability.

Suggested change

fName, err := batch.download(ctx, filepath.Join(prefix, StatusTrackerFileName))

statusTrackerFile, err := batch.download(ctx, filepath.Join(prefix, StatusTrackerFileName))

Usually within a function name which clearly defines the intent, we can shorten the name to indicate popular structure. I like to indicate files through f etc. Wdyt ?

saurav-malani · 2022-10-26T15:17:20Z

regulation-worker/internal/delete/batch/batch.go

+	switch destType {
+	case "S3":
+		return map[string]filehandler.LocalFileHandler{
+			".json.gz": filehandler.NewGZIPLocalFileHandler(filehandler.CamelCase),


Given that we anyways support handling parquet file. And, any file type could we stored in S3, won't it better to add all the supported filehandler for all destination.

Let me think about it. That can be done.

saurav-malani · 2022-10-26T15:36:33Z

regulation-worker/internal/delete/batch/filehandler/gzip.go

+	return nil
+}
+
+func (h *GZIPLocalFileHandler) RemoveIdentity(ctx context.Context, attributes []model.User) error {


Given that we have changed the logic of RemoveIdentity and since this is going to be one of the most time consuming part of the loop during deletion. I strongly feel we should benchmark the old & new deletion logic.

I have a feeling that if sed knows about all the users that needs to be matched for, it will do some kind of optimization do avoid literally iterating over each OR condition of pattern file and look for a match in each line.

Based on the man of sed, -f is just reading the commands from the file and appending it to the list of commands to sed. -e which we have replaced it with does the same thing as well.

The main approach we have changed is letting sed completely handle the file from which the data needs to be removed and we are doing it in memory. This is something which needs to be benchmarked and checked against.

sundernagesh18 · 2022-11-01T07:23:37Z

This feature is good to go. QA Approved.

… file

…ocessing

…t accidental matches

…andle

abhimanyubabbar force-pushed the feat.s3-datalake-regulation branch from dd328aa to 1816184 Compare October 1, 2022 06:52

github-actions bot added server-team with tests labels Oct 1, 2022

abhimanyubabbar changed the title ~~Added base support for s3 datalake deletion~~ feat(warehouse): Added base support for s3 datalake deletion as part of regulation API. Oct 1, 2022

abhimanyubabbar changed the title ~~feat(warehouse): Added base support for s3 datalake deletion as part of regulation API.~~ feat(warehouse): added base support for s3 datalake deletion as part of regulation API. Oct 1, 2022

abhimanyubabbar force-pushed the feat.s3-datalake-regulation branch from 02d9940 to f39e5b9 Compare October 17, 2022 11:14

abhimanyubabbar marked this pull request as ready for review October 17, 2022 11:14

abhimanyubabbar force-pushed the feat.s3-datalake-regulation branch from f39e5b9 to 411b2ba Compare October 17, 2022 11:16

lvrach requested review from lvrach, saurav-malani and a team October 17, 2022 11:23

lvrach reviewed Oct 17, 2022

View reviewed changes

achettyiitr reviewed Oct 19, 2022

View reviewed changes

lvrach approved these changes Oct 25, 2022

View reviewed changes

saurav-malani reviewed Oct 26, 2022

View reviewed changes

abhimanyubabbar force-pushed the feat.s3-datalake-regulation branch from fa1fe1a to f0aff27 Compare October 27, 2022 05:58

saurav-malani approved these changes Oct 31, 2022

View reviewed changes

abhimanyubabbar force-pushed the feat.s3-datalake-regulation branch from 0dd3a12 to bb97d4e Compare November 1, 2022 08:11

abhimanyubabbar added 9 commits November 1, 2022 14:23

Added base support for s3 datalake deletion

3b22c57

Linting Fixes

aeacfa6

Added more test cases

bad9d49

refactoring the structure to be more modular

1e7a6ab

fixing the process to fetch already cleaned files from status tracker…

1a4dffb

… file

fix the call to calculate the max routines

1893e1e

Linting fixes

7ed988d

fixed call to list files under prefix only

56b673a

Fixed a potential lock issue when unable to locate filehandler for pr…

91dea4d

…ocessing

abhimanyubabbar added 11 commits November 1, 2022 14:23

Lint fixes

3178090

Fixed build issue

2544737

Added support for escaping special characters in the userId to preven…

78e5342

…t accidental matches

Reduce the timing

dc10379

create a new file handler everytime we have a corresponding file to h…

699c6ce

…andle

Linting fix

db688f6

Added support to read the interval between loops from env

2297a44

generate sed pattern only once for gzip filtering

ed02621

Remove unused code

9ea7f87

fixed deepsource comments

cf13846

remove unused test case

d5fa7dd

abhimanyubabbar force-pushed the feat.s3-datalake-regulation branch from bb97d4e to d5fa7dd Compare November 1, 2022 09:38

abhimanyubabbar merged commit 4369abf into master Nov 1, 2022

This was referenced Nov 1, 2022

chore: release 1.3.0 #2562

Closed

chore: release 1.3.0 #2665

Merged

chore: prerelease 1.3.0-rc.1 #2666

Merged

This was referenced Nov 8, 2022

chore: prerelease 1.3.0-rc.2 #2673

Merged

chore: prerelease 1.3.0-rc.3 #2681

Merged

chore: release 1.3.0 #2695

Closed

chore: prerelease 1.4.0-rc.1 #2696

Closed

This was referenced Dec 1, 2022

chore: release 1.3.0 #2749

Closed

chore: prerelease 1.4.0-rc.1 #2750

Merged

chore: release 1.4.0 #2752

Merged

github-actions bot deleted the feat.s3-datalake-regulation branch January 2, 2023 02:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(warehouse): added base support for s3 datalake deletion as part of regulation API. #2515

feat(warehouse): added base support for s3 datalake deletion as part of regulation API. #2515

abhimanyubabbar commented Sep 30, 2022 •

edited

Loading

codecov bot commented Oct 17, 2022 •

edited

Loading

lvrach left a comment

lvrach Oct 17, 2022

abhimanyubabbar Oct 18, 2022

abhimanyubabbar Oct 18, 2022

achettyiitr Oct 19, 2022

abhimanyubabbar Oct 19, 2022

saurav-malani Oct 26, 2022

abhimanyubabbar Oct 27, 2022

saurav-malani Oct 26, 2022 •

edited

Loading

saurav-malani Oct 26, 2022

abhimanyubabbar Oct 28, 2022

saurav-malani Oct 26, 2022

saurav-malani Oct 26, 2022

abhimanyubabbar Oct 28, 2022

saurav-malani Oct 26, 2022

abhimanyubabbar Oct 28, 2022 •

edited

Loading

abhimanyubabbar Oct 28, 2022

abhimanyubabbar Oct 28, 2022

saurav-malani Oct 26, 2022

saurav-malani Oct 26, 2022

abhimanyubabbar Oct 28, 2022

saurav-malani Oct 26, 2022

abhimanyubabbar Oct 28, 2022

saurav-malani Oct 26, 2022

abhimanyubabbar Oct 28, 2022

saurav-malani Oct 26, 2022

abhimanyubabbar Oct 28, 2022

sundernagesh18 commented Nov 1, 2022

	if err := f.Close(); err != nil {
	return nil, fmt.Errorf("closing file: %w", err)
	}

	pkgLogger.Infof("successfully completed loop of ")
	pkgLogger.Infof("successfully completed loop")

	fName, err := batch.download(ctx, filepath.Join(prefix, StatusTrackerFileName))
	statusTrackerFile, err := batch.download(ctx, filepath.Join(prefix, StatusTrackerFileName))

feat(warehouse): added base support for s3 datalake deletion as part of regulation API. #2515

feat(warehouse): added base support for s3 datalake deletion as part of regulation API. #2515

Conversation

abhimanyubabbar commented Sep 30, 2022 • edited Loading

Description

Notion Ticket

Security

codecov bot commented Oct 17, 2022 • edited Loading

Codecov Report

lvrach 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

saurav-malani Oct 26, 2022 • edited Loading

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

abhimanyubabbar Oct 28, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sundernagesh18 commented Nov 1, 2022

abhimanyubabbar commented Sep 30, 2022 •

edited

Loading

codecov bot commented Oct 17, 2022 •

edited

Loading

saurav-malani Oct 26, 2022 •

edited

Loading

abhimanyubabbar Oct 28, 2022 •

edited

Loading