chore(warehouse): added warehouse handling for s3 with glue and other improvements #2940

achettyiitr · 2023-02-05T14:06:55Z

Description

Added S3 Glue validations.
Use the namespace defined by the customer in the destination config. If not present use the default test namespace.
Don't use random tables every time while doing validations.
Using simple components like objectStorage, connections, createSchema, createAlterTable, fetchSchema, loadTable to satisfy Validate() error instead of coupling it with validation steps.

Improvements

Avoid coupling the GRPC module with the validations module.
Test coverage for the validations module.

Design changes

Notion Ticket

Security

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

… improvements

codecov · 2023-02-06T02:16:35Z

Codecov Report

Patch coverage: 72.47% and project coverage change: +0.33 🎉

Comparison is base (4832630) 52.77% compared to head (54a8a5d) 53.11%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2940      +/-   ##
==========================================
+ Coverage   52.77%   53.11%   +0.33%     
==========================================
  Files         334      332       -2     
  Lines       52216    52313      +97     
==========================================
+ Hits        27558    27784     +226     
+ Misses      23056    22937     -119     
+ Partials     1602     1592      -10

Impacted Files	Coverage Δ
warehouse/admin.go	`3.17% <0.00%> (+0.14%)`	⬆️
warehouse/api.go	`69.89% <0.00%> (+0.20%)`	⬆️
warehouse/integrations/bigquery/bigquery.go	`2.08% <0.00%> (+<0.01%)`	⬆️
warehouse/integrations/testhelper/setup.go	`0.00% <0.00%> (ø)`
warehouse/upload.go	`23.31% <0.00%> (+0.05%)`	⬆️
warehouse/utils/utils.go	`70.07% <ø> (ø)`
warehouse/integrations/redshift/redshift.go	`10.34% <26.66%> (+0.49%)`	⬆️
warehouse/warehousegrpc.go	`70.42% <41.30%> (-9.23%)`	⬇️
warehouse/integrations/deltalake/deltalake.go	`65.17% <73.68%> (+0.06%)`	⬆️
warehouse/validations/validate.go	`78.90% <79.94%> (+27.30%)`	⬆️
... and 38 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.

…s/rudder-server into chore.validations-improvements

achettyiitr · 2023-02-07T04:42:56Z

warehouse/integrations/bigquery/bigquery.go

@@ -115,23 +115,25 @@ func (bq *HandleT) DeleteTable(tableName string) (err error) {
 	return
 }

-func (bq *HandleT) CreateTable(tableName string, columnMap map[string]string) (err error) {
+func (bq *HandleT) CreateTable(tableName string, columnMap map[string]string) error {


While writing the Unit test, we found that suppression is not happening properly.
The error is getting overridden when we createView if dedup flag is enabled.

achettyiitr · 2023-02-07T04:44:17Z

warehouse/integrations/deltalake/deltalake.go

+	}
+
+	// Handle error in case of single column
+	if len(columnsInfo) == 1 {


We would need to suppress the errors in case of a single column alter since databricks does support ADD COLUMN IF NOT EXISTS

achettyiitr · 2023-02-07T04:46:18Z

warehouse/validations/steps.go

-		&validationStep{
-			ID:        2,
-			Name:      verifyingConnections,
-			Validator: ct.verifyingConnections,


There was too much coupling with steps and validation functions. steps API will return only the steps. Once we get the steps, we call NewValidator to get the validating function.

achettyiitr · 2023-02-07T04:47:20Z

warehouse/validations/validations_suite_test.go

@@ -1,13 +0,0 @@
-package validations_test


Since the package does not contain Ginkgo-based tests. Removing it.

achettyiitr · 2023-02-07T04:47:56Z

warehouse/validations/validations.go

 	return map[string]*validationFunc{
 		"validate": {
-			Path: "/validate",


The Path is not being used. Removed it.

achettyiitr · 2023-02-07T04:51:13Z

warehouse/warehousegrpc.go

@@ -94,12 +97,49 @@ func (*warehouseGRPC) TriggerWHUpload(_ context.Context, request *proto.WHUpload
 	return res, err
 }

-func (grpc *warehouseGRPC) Validate(_ context.Context, req *proto.WHValidationRequest) (*proto.WHValidationResponse, error) {
-	handleT := validations.CTHandleT{


The domain model for the validations package should depend on the destination only, not on the internal representation of the destination like whether tunneling is enabled or not.
Moving the logic to the caller itself

…re.validations-improvements

achettyiitr · 2023-02-16T06:15:46Z

warehouse/integrations/redshift/redshift.go

+			if CheckAndIgnoreColumnAlreadyExistError(err) {
+				pkgLogger.Infof("RS: Column already exists for destinationID: %s, tableName: %s with query: %v", rs.Warehouse.Destination.ID, tableName, query)


We would need to suppress the errors in case of a single column alter since Redshift does support ADD COLUMN IF NOT EXISTS

achettyiitr · 2023-02-16T06:25:19Z

warehouse/validations/helper.go

@@ -1,60 +0,0 @@
-package validations


Moved to validate.go

achettyiitr · 2023-02-16T06:25:26Z

warehouse/validations/types.go

@@ -1,149 +0,0 @@
-package validations


Moved to validate.go

achettyiitr · 2023-02-16T06:26:24Z

warehouse/validations/validate.go

+type objectStorage struct {
+	destination *backendconfig.DestinationT
+}
+
+type connections struct {
+	manager     manager.WarehouseOperations
+	destination *backendconfig.DestinationT
+}
+
+type createSchema struct {
+	manager manager.WarehouseOperations
+}
+
+type createAlterTable struct {
+	manager manager.WarehouseOperations
+	table   string
+}
+
+type fetchSchema struct {
+	manager     manager.WarehouseOperations
+	destination *backendconfig.DestinationT
+}
+
+type loadTable struct {
+	manager     manager.WarehouseOperations
+	destination *backendconfig.DestinationT
+	table       string
+}


These satisfies the validator interface to validate the particular step.

lvrach · 2023-02-16T10:53:35Z

warehouse/validations/validate.go

+	if err = operations.Setup(warehouse, &mockUploader{
+		dest: dest,
+	}); err != nil {


I am not sure why we are doing things. Since mockUploader is usually for helping testing doesn't imply a lot about its functionality.

Because of the complexity of the changes associated with Uploader interface in #2993, I am thinking of working on it separately.
I renamed it to dummyUploader for the current use case.

…re.validations-improvements

warehouse/validations/validate.go

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

chore(warehouse): added warehouse handling for s3 with glue and other…

e4f25e1

… improvements

achettyiitr force-pushed the chore.validations-improvements branch from 2e89fe0 to e4f25e1 Compare February 5, 2023 14:59

achettyiitr added 2 commits February 5, 2023 21:44

suppress add column already exists for redshift

02c8bab

added test case for redshift check column already exists

e8d4a4e

achettyiitr force-pushed the chore.validations-improvements branch from f09e02d to e8d4a4e Compare February 6, 2023 01:52

achettyiitr and others added 5 commits February 6, 2023 16:17

added deltalake single column failure handling

cdff621

bigquery check and ignore already exists error

c3cdce7

Merge branch 'master' into chore.validations-improvements

5616747

handler manager error

accf230

Merge branch 'chore.validations-improvements' of github.com:rudderlab…

1c8218f

…s/rudder-server into chore.validations-improvements

achettyiitr requested review from lvrach and abhimanyubabbar February 6, 2023 15:25

achettyiitr added 2 commits February 7, 2023 00:49

some more tests

52f7625

master pull

e3ea45f

achettyiitr commented Feb 7, 2023

View reviewed changes

achettyiitr added 2 commits February 7, 2023 10:35

deltalake test fix

abb9d27

Merge branch 'master' into chore.validations-improvements

3c5e7ab

achettyiitr mentioned this pull request Feb 9, 2023

fix(warehouse): deltalake client crashing when failing on connectionstep #2961

Merged

1 task

achettyiitr added 4 commits February 9, 2023 22:51

master pull

4a9c018

master pull

fefad05

Merge branch 'master' into chore.validations-improvements

1374522

added some more tests.

7c6b4ea

achettyiitr added 2 commits February 15, 2023 14:12

deltalake add column fix

fdd600d

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

948ee18

…re.validations-improvements

achettyiitr commented Feb 16, 2023

View reviewed changes

warehouse/validations/helper.go

@@ -1,60 +0,0 @@

package validations

Copy link

Member Author

achettyiitr Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to validate.go

achettyiitr commented Feb 16, 2023

View reviewed changes

warehouse/validations/types.go

@@ -1,149 +0,0 @@

package validations

Copy link

Member Author

achettyiitr Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to validate.go

achettyiitr commented Feb 16, 2023

View reviewed changes

some more changes

d501271

lvrach reviewed Feb 16, 2023

View reviewed changes

achettyiitr requested a review from lvrach February 20, 2023 11:47

achettyiitr added 2 commits February 22, 2023 13:57

master pull

0eca54a

added json logs

f6386a5

achettyiitr requested review from cisse21 and removed request for abhimanyubabbar February 22, 2023 12:19

achettyiitr added 5 commits February 22, 2023 18:20

deepsource changes

6ec879e

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

387c729

…re.validations-improvements

fix steps test

baee5c1

Merge branch 'master' into chore.validations-improvements

9130a8d

master pull

9be095b

lvrach approved these changes Feb 28, 2023

View reviewed changes

cisse21 reviewed Mar 2, 2023

View reviewed changes

warehouse/validations/validate.go Show resolved Hide resolved

chore: import cleanup

54a8a5d

cisse21 approved these changes Mar 2, 2023

View reviewed changes

cisse21 merged commit 3495797 into master Mar 2, 2023

cisse21 deleted the chore.validations-improvements branch March 2, 2023 10:00

This was referenced Mar 6, 2023

chore: prerelease 1.7.0-preview.2 #3071

Merged

chore: prerelease 1.7.0-preview.3 #3084

Merged

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

chore(warehouse): added warehouse handling for s3 with glue and other improvements #2940

chore(warehouse): added warehouse handling for s3 with glue and other improvements #2940

achettyiitr commented Feb 5, 2023 •

edited

Loading

codecov bot commented Feb 6, 2023 •

edited

Loading

achettyiitr Feb 7, 2023 •

edited

Loading

achettyiitr Feb 7, 2023

achettyiitr Feb 7, 2023

achettyiitr Feb 7, 2023

achettyiitr Feb 7, 2023

achettyiitr Feb 7, 2023 •

edited

Loading

achettyiitr Feb 16, 2023

achettyiitr Feb 16, 2023

achettyiitr Feb 16, 2023

achettyiitr Feb 16, 2023

lvrach Feb 16, 2023

achettyiitr Feb 22, 2023 •

edited

Loading

		if CheckAndIgnoreColumnAlreadyExistError(err) {
		pkgLogger.Infof("RS: Column already exists for destinationID: %s, tableName: %s with query: %v", rs.Warehouse.Destination.ID, tableName, query)

chore(warehouse): added warehouse handling for s3 with glue and other improvements #2940

chore(warehouse): added warehouse handling for s3 with glue and other improvements #2940

Conversation

achettyiitr commented Feb 5, 2023 • edited Loading

Description

Improvements

Design changes

Notion Ticket

Security

codecov bot commented Feb 6, 2023 • edited Loading

Codecov Report

achettyiitr Feb 7, 2023 • 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

achettyiitr Feb 7, 2023 • 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

achettyiitr Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

achettyiitr commented Feb 5, 2023 •

edited

Loading

codecov bot commented Feb 6, 2023 •

edited

Loading

achettyiitr Feb 7, 2023 •

edited

Loading

achettyiitr Feb 7, 2023 •

edited

Loading

achettyiitr Feb 22, 2023 •

edited

Loading