fix(warehouse): infinites retries #3050

achettyiitr · 2023-02-28T13:53:27Z

Description

Since we already had a hard limit for x Hours and y Times we don’t need hasAllTablesSkipped
- The problem with having hasAllTablesSkipped is it is being shared across multiple goroutines during exporting (regular tables, identifies/users tables, ID resolution)
- With the bug retries causing succeeded tables to get retried again, during export, the oldest exporting goroutines set it to true in case of successful exports for that goroutine.
- Later this is used to decide whether we should neglect to mark the upload status as aborted.
Once the failed sync is retried, all succeeded tables are getting retries.
- We used to set tableUploadStatuses once. But this check does not become invalid because we are initializing it with an empty slice.

Notion Ticket

https://www.notion.so/rudderstacks/infinite-retries-a61e259c0b9349bfb5485083298d6baf?pvs=4

Security

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

… of retries and retry duration

….infinite-retrying

codecov · 2023-03-02T12:13:27Z

Codecov Report

Patch coverage: 48.97% and project coverage change: +0.13 🎉

Comparison is base (1d9f63b) 53.43% compared to head (a8de1fe) 53.56%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3050      +/-   ##
==========================================
+ Coverage   53.43%   53.56%   +0.13%     
==========================================
  Files         342      346       +4     
  Lines       52747    53805    +1058     
==========================================
+ Hits        28185    28821     +636     
- Misses      22947    23349     +402     
- Partials     1615     1635      +20

Impacted Files	Coverage Δ
utils/misc/misc.go	`51.37% <ø> (-0.49%)`	⬇️
warehouse/identities.go	`1.04% <0.00%> (-0.04%)`	⬇️
warehouse/identity/identity.go	`0.51% <0.00%> (+0.06%)`	⬆️
...ehouse/integrations/azure-synapse/azure-synapse.go	`0.31% <0.00%> (+0.02%)`	⬆️
warehouse/integrations/postgres/postgres.go	`0.53% <0.00%> (-28.10%)`	⬇️
warehouse/internal/model/upload.go	`100.00% <ø> (ø)`
warehouse/internal/service/recovery.go	`100.00% <ø> (ø)`
warehouse/integrations/mssql/mssql.go	`1.62% <6.20%> (+0.66%)`	⬆️
warehouse/upload.go	`20.13% <11.37%> (-3.47%)`	⬇️
warehouse/integrations/postgres-legacy/postgres.go	`28.62% <28.62%> (ø)`
... and 11 more

... and 10 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

achettyiitr · 2023-03-02T14:59:54Z

warehouse/upload.go

-// TableSkipError is a custom error type to capture if a table load is skipped because of a previously failed table load
-type TableSkipError struct {
-	tableName        string
-	previousJobID    int64
-	previousJobError string
-}
-
-func (tse *TableSkipError) Error() string {
-	return fmt.Sprintf("Skipping %s table because it previously failed to load in an earlier job: %d with error: %s", tse.tableName, tse.previousJobID, tse.previousJobError)
-}
-


Removing this as we are now avoiding TableSkipError.

lvrach · 2023-03-07T10:46:53Z

warehouse/upload_test.go

@@ -6,20 +6,20 @@ import (
 	"fmt"
 	"os"
 	"testing"
+	time2 "time"

 	"github.com/aws/smithy-go/time"


why are we using this package for our tests?

Let me remove this. Not sure why we are using this.

lvrach · 2023-03-09T08:00:06Z

warehouse/upload.go

-			panic(err)
+		// Previous upload and table upload failed
+		exportingfailedStatus = func(status string) bool {
+			return status == TableUploadExportingFailed || status == UserTableUploadExportingFailed || status == IdentityTableUploadExportingFailed


[minor] & opinionated

Suggested change

return status == TableUploadExportingFailed || status == UserTableUploadExportingFailed || status == IdentityTableUploadExportingFailed

switch status {

case TableUploadExportingFailed, UserTableUploadExportingFailed, IdentityTableUploadExportingFailed:

return true

default:

return false

}

We should consider moving this logic outside of this function. Maybe it could be part of the table upload model, or introduce a separate failure flag.

The concern I have is when we introduce a new failed state we might forget to add it here.

Looks like the other 2 flags were not being used. Removing this.

achettyiitr · 2023-03-09T10:15:02Z

warehouse/internal/repo/upload_test.go

+	_, err := db.Exec(`INSERT INTO wh_uploads (id, destination_id, source_id, in_progress, destination_type, status, namespace, schema, created_at, updated_at)
+		VALUES
+		(1, 1, 1, true, 'RS', 'exporting_data', 'test_namespace', '{}', NOW(), NOW()),
+		(2, 1, 1, true, 'RS', 'aborted', 'test_namespace', '{}', NOW(), NOW())
+	`)
+	require.NoError(t, err)
+
+	_, err = db.Exec(`INSERT INTO wh_table_uploads (id, wh_upload_id, table_name, status, error, created_at, updated_at)
+		VALUES
+		(1, 1, 'test_table_1','exporting_data', '{}', NOW(), NOW()),
+		(2, 1, 'test_table_2','exporting_data_failed', 'error loading data', NOW(), NOW())
+	`)
+	require.NoError(t, err)


This has be modified with repository in this #3087

….infinite-retrying

achettyiitr added 3 commits February 28, 2023 10:35

removed hasAllTablesSkipped since we already have limit around number…

04312e6

… of retries and retry duration

added support for table uploads repo

4395397

Merge branch 'master' of github.com:rudderlabs/rudder-server into fix…

2878397

….infinite-retrying

github-actions bot added warehouse-team with tests labels Feb 28, 2023

achettyiitr requested review from lvrach and cisse21 February 28, 2023 13:53

make fmt

4d7fbfa

achettyiitr marked this pull request as draft March 1, 2023 06:12

achettyiitr added 6 commits March 1, 2023 16:27

Merge branch 'master' of github.com:rudderlabs/rudder-server into fix…

dfde01a

….infinite-retrying

added test cases for Table Uploads

2798ad5

Merge branch 'master' of github.com:rudderlabs/rudder-server into fix…

d157f5f

….infinite-retrying

make fmt

ffed595

reverted table repo changes

226380a

fix tests

9d121cd

achettyiitr marked this pull request as ready for review March 2, 2023 11:28

added test cases

7eccaaf

achettyiitr commented Mar 2, 2023

View reviewed changes

achettyiitr added 4 commits March 6, 2023 09:45

Added test case for job.Aborted

ce25fd6

added test cases for table uploads

2823baf

fix test failures

411d406

master pull

e0a2216

achettyiitr force-pushed the fix.infinite-retrying branch from 102e08b to e0a2216 Compare March 7, 2023 07:01

lvrach reviewed Mar 7, 2023

View reviewed changes

achettyiitr requested a review from lvrach March 7, 2023 13:45

achettyiitr changed the title ~~fix(warehouse): infinites retries~~ fix(warehouse): infinites retries and table uploads repository Mar 7, 2023

achettyiitr added 2 commits March 9, 2023 01:26

master pull

52ef709

review comments

77b103f

achettyiitr force-pushed the fix.infinite-retrying branch from f4b6b21 to 77b103f Compare March 8, 2023 20:01

achettyiitr changed the title ~~fix(warehouse): infinites retries and table uploads repository~~ fix(warehouse): infinites retries Mar 8, 2023

lvrach approved these changes Mar 9, 2023

View reviewed changes

achettyiitr commented Mar 9, 2023

View reviewed changes

achettyiitr added 3 commits March 9, 2023 15:46

Merge branch 'master' of github.com:rudderlabs/rudder-server into fix…

d39bc9f

….infinite-retrying

review comments

5d66c65

feat(warehouse): table uploads repository (#3085)

a8de1fe

cisse21 approved these changes Mar 10, 2023

View reviewed changes

cisse21 merged commit d3320fa into master Mar 10, 2023

cisse21 deleted the fix.infinite-retrying branch March 10, 2023 07:42

This was referenced Mar 14, 2023

chore: prerelease 1.7.0-preview.4 #3099

Merged

chore: release 1.7.0 #3106

Merged

chore: prerelease 1.7.0-rc.1 #3107

Merged

chore: prerelease 1.7.0-rc.2 #3114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(warehouse): infinites retries #3050

fix(warehouse): infinites retries #3050

achettyiitr commented Feb 28, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading

achettyiitr Mar 2, 2023

lvrach Mar 7, 2023

achettyiitr Mar 7, 2023

lvrach Mar 9, 2023

lvrach Mar 9, 2023

achettyiitr Mar 9, 2023

achettyiitr Mar 9, 2023

-			return status == TableUploadExportingFailed || status == UserTableUploadExportingFailed || status == IdentityTableUploadExportingFailed
+                         switch status {
+                         case TableUploadExportingFailed, UserTableUploadExportingFailed, IdentityTableUploadExportingFailed:
+                             return true
+                          default:
+                             return false
+                          }

fix(warehouse): infinites retries #3050

fix(warehouse): infinites retries #3050

Conversation

achettyiitr commented Feb 28, 2023 • edited Loading

Description

Notion Ticket

Security

codecov bot commented Mar 2, 2023 • edited Loading

Codecov Report

achettyiitr Mar 2, 2023

Choose a reason for hiding this comment

lvrach Mar 7, 2023

Choose a reason for hiding this comment

achettyiitr Mar 7, 2023

Choose a reason for hiding this comment

lvrach Mar 9, 2023

Choose a reason for hiding this comment

lvrach Mar 9, 2023

Choose a reason for hiding this comment

achettyiitr Mar 9, 2023

Choose a reason for hiding this comment

achettyiitr Mar 9, 2023

Choose a reason for hiding this comment

achettyiitr commented Feb 28, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading