chore: move warehouse handle to router #3687

achettyiitr · 2023-07-31T10:02:51Z

Description

Rename HandleT to Router and move in its self-defined file.
Also, renamed identifies.go and scheduling.go into their router_identifies.go and router_scheduling.go router scope.
Removed global configurations for Router.
Added test cases for Router [84% coverage]
Rewrote test for router_scheduling and added some more test cases.

Notion Ticket

https://www.notion.so/rudderstacks/Move-warehouseHandleT-to-rotuer-354c3e45fc674c3e988d1a79d244a5ff?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.

warehouse/router_scheduling.go

fracasula · 2023-08-07T15:43:25Z

warehouse/router_identities.go

@@ -152,16 +149,16 @@ func (wh *HandleT) hasLocalIdentityData(warehouse model.Warehouse) (exists bool)
 `,
 		warehouseutils.IdentityMergeRulesTableName(warehouse),
 	)
-	err := wh.dbHandle.QueryRow(sqlStatement).Scan(&exists)
+	err := r.dbHandle.QueryRow(sqlStatement).Scan(&exists)
 	if err != nil {
 		// TODO: Handle this
 		panic(fmt.Errorf("Query: %s\nfailed with Error : %w", sqlStatement, err))


For my information, how come we panic here? Can't we handle it?

P.S.: the naked return feels harder to understand in this case, I suggest we use normal returns if you're OK with it as well.

A follow up task can also be created to handle this panic and possibly other ones too, but first it'd be nice to understand the scope of this.

achettyiitr · 2023-08-07T15:58:42Z

warehouse/warehouse_test.go

@@ -72,121 +54,3 @@ func getMockStats(g GinkgoTInterface) (*mock_stats.MockStats, *mock_stats.MockMe
 	mockMeasurement := mock_stats.NewMockMeasurement(ctrl)
 	return mockStats, mockMeasurement
 }
-
-func TestUploadJob_ProcessingStats(t *testing.T) {


This is moved to Router Processor Stats test.

achettyiitr · 2023-08-07T15:59:38Z

warehouse/scheduling_test.go

@@ -1,136 +0,0 @@
-package warehouse_test


This is moved to router_scheduling_test.go

fracasula

Still reviewing, posting a few questions in the meantime.

warehouse/internal/repo/upload_test.go

fracasula · 2023-08-07T16:27:40Z

warehouse/router_identities.go

-func (wh *HandleT) hasWarehouseData(ctx context.Context, warehouse model.Warehouse) (bool, error) {
-	whManager, err := manager.New(wh.destType, wh.conf, wh.Logger, wh.stats)
+func (r *Router) hasWarehouseData(ctx context.Context, warehouse model.Warehouse) (bool, error) {
+	whManager, err := manager.New(r.destType, r.conf, r.logger, r.stats)


We might not need to create a new instance each time.

This is used in other places too so it might make sense to create it only once (see line 399).

fracasula · 2023-08-07T16:27:54Z

warehouse/router_identities.go

-func (wh *HandleT) hasWarehouseData(ctx context.Context, warehouse model.Warehouse) (bool, error) {
-	whManager, err := manager.New(wh.destType, wh.conf, wh.Logger, wh.stats)
+func (r *Router) hasWarehouseData(ctx context.Context, warehouse model.Warehouse) (bool, error) {
+	whManager, err := manager.New(r.destType, r.conf, r.logger, r.stats)
 	if err != nil {
 		panic(err)


Why not return an error here?

fracasula · 2023-08-07T16:31:28Z

warehouse/router_identities.go

@@ -173,10 +170,10 @@ func (wh *HandleT) hasWarehouseData(ctx context.Context, warehouse model.Warehou
 	return !empty, nil
 }

-func (wh *HandleT) setupIdentityTables(ctx context.Context, warehouse model.Warehouse) {
+func (r *Router) setupIdentityTables(ctx context.Context, warehouse model.Warehouse) {


Wouldn't it make sense to create the table and its indexes in the same transaction instead of panicking each time with the risk of leaving the database in an "incomplete" state?

Just asking btw, not saying we have to address this in this PR but let's discuss if needed.

fracasula · 2023-08-07T16:36:08Z

warehouse/router_identities.go

-			pkgLogger.Infof("[WH]: Did not find local identity tables..")
-			pkgLogger.Infof("[WH]: Generating identity tables based on data in warehouse %s:%s", wh.destType, warehouse.Destination.ID)
-			upload = wh.initPrePopulateDestIdentitiesUpload(warehouse)
+			r.logger.Infof("[WH]: Did not find local identity tables..")


Suggested change

r.logger.Infof("[WH]: Did not find local identity tables..")

fracasula · 2023-08-07T16:38:39Z

warehouse/router_identities.go

@@ -441,15 +438,15 @@ func (wh *HandleT) populateHistoricIdentities(ctx context.Context, warehouse mod

 		err = job.schemaHandle.fetchSchemaFromWarehouse(ctx, whManager)
 		if err != nil {
-			pkgLogger.Errorf(`[WH]: Failed fetching schema from warehouse: %v`, err)
+			r.logger.Errorf(`[WH]: Failed fetching schema from warehouse: %v`, err)
 			job.setUploadError(err, model.Aborted)


How come we are ignoring this error? We're doing the same with other calls to setUploadError or setUploadStatus.

warehouse/router_tracker.go

warehouse/router.go

…ments' into chore.whBackendConfig+warehouse-handle-to-router

achettyiitr · 2023-08-08T11:43:43Z

@fracasula Added a task for addressing the review comments for router_identifies.
Once this is merged, will make it in a separate PR.

lvrach · 2023-08-08T12:52:57Z

warehouse/router_identities.go

I have noticed we are using fmt.Sprintf for providing argument into to the SQL query, we should use prepared statements.

fracasula

We can have a separate task for the prepared statements.

fracasula added 30 commits July 26, 2023 09:24

$@fracasula$

chore: typo

b1b0c1b

$@fracasula$

chore: backend-config manager

fbc204f

$@fracasula$

fix: retry

ba0427d

$@fracasula$

fix: api

1deb4ac

$@fracasula$

fix: admin

58f4dfa

$@fracasula$

chore: updating tests

451348c

$@fracasula$

chore: removing unused attributes

87ff175

$@fracasula$

chore: bigquery test fix

f47aaab

$@fracasula$

chore: linting

301e17a

$@fracasula$

chore: moving constructor

0bc99d5

$@fracasula$

chore: refactor wh grpc test

44aeb08

$@fracasula$

chore: removing duplicated code in test

7b95baa

$@fracasula$

chore: cleaning up

cdd1bc8

$@fracasula$

chore: ssh tunneling and logger

b8d36a0

$@fracasula$

chore: removing unused client

8dd4296

$@fracasula$

chore: better routine management

ccf5134

$@fracasula$

chore: rename variable

a5e75a2

$@fracasula$

chore: removing TODOs

7e3d25f

$@fracasula$

chore: more efficient workerIdentifier()

2a30339

$@fracasula$

chore: adding TODOs

c70b229

$@fracasula$

chore: adding info to backendConfigSubscriber

d79319d

$@fracasula$

chore: optimizing concurrent code

39cd70b

$@fracasula$

chore: using logger

0ad24f7

$@fracasula$

chore: initialConfigFetched

ee7b1e1

$@fracasula$

chore: moving var

5c756c7

$@fracasula$

chore: TODO

642dd40

$@fracasula$

chore: wrapped db handle

b24e638

$@fracasula$

chore: moving workerChannelMap initialization

077abc3

$@fracasula$

chore: removing unused field

8f9f564

$@fracasula$

chore: removing unused variable

009d557

$fracasula$

fracasula reviewed Aug 7, 2023

View reviewed changes

warehouse/router_scheduling.go Outdated Show resolved Hide resolved

$fracasula$

fracasula reviewed Aug 7, 2023

View reviewed changes

add test for uploads repo reset in progress

c8ea735

achettyiitr commented Aug 7, 2023

View reviewed changes

warehouse/scheduling_test.go

@@ -1,136 +0,0 @@

package warehouse_test

Copy link

Member Author

achettyiitr Aug 7, 2023

Choose a reason for hiding this comment

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

This is moved to router_scheduling_test.go

$fracasula$

fracasula reviewed Aug 7, 2023

View reviewed changes

achettyiitr force-pushed the chore.whBackendConfig+warehouse-handle-to-router branch from f0cde37 to 0fbd947 Compare August 8, 2023 00:11

chore: added scheduling tests.

53a1f6d

achettyiitr force-pushed the chore.whBackendConfig+warehouse-handle-to-router branch from 0fbd947 to 53a1f6d Compare August 8, 2023 00:25

$fracasula$

fracasula reviewed Aug 8, 2023

View reviewed changes

warehouse/router_tracker.go Outdated Show resolved Hide resolved

$fracasula$

fracasula reviewed Aug 8, 2023

View reviewed changes

warehouse/router.go Outdated Show resolved Hide resolved

chore: some more changes

3fd347b

$fracasula$

fracasula reviewed Aug 8, 2023

View reviewed changes

warehouse/router.go Outdated Show resolved Hide resolved

achettyiitr added 3 commits August 8, 2023 14:03

chore: comments for tracker

4f00a9d

chore: review comments

fbbc869

Merge remote-tracking branch 'origin/chore.whBackendConfig+review-com…

f6cb961

…ments' into chore.whBackendConfig+warehouse-handle-to-router

achettyiitr force-pushed the chore.whBackendConfig+warehouse-handle-to-router branch from d84af52 to f6cb961 Compare August 8, 2023 08:46

chore: review comments

867d658

achettyiitr requested a review from fracasula August 8, 2023 11:43

achettyiitr marked this pull request as ready for review August 8, 2023 12:49

lvrach reviewed Aug 8, 2023

View reviewed changes

$fracasula$

fracasula approved these changes Aug 8, 2023

View reviewed changes

achettyiitr requested a review from lvrach August 8, 2023 17:29

lvrach approved these changes Aug 9, 2023

View reviewed changes

achettyiitr merged commit e227046 into master Aug 9, 2023
36 checks passed

achettyiitr deleted the chore.whBackendConfig+warehouse-handle-to-router branch August 9, 2023 07:23

This was referenced Aug 9, 2023

chore: prerelease 1.12.0-rc.1 #3727

Merged

chore: prerelease 1.12.0-rc.2 #3738

Merged

devops-github-rudderstack mentioned this pull request Aug 14, 2023

chore: release 1.12.0 #3747

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move warehouse handle to router #3687

chore: move warehouse handle to router #3687

achettyiitr commented Jul 31, 2023 •

edited

$@fracasula$ fracasula Aug 7, 2023 •

edited

$@fracasula$ fracasula Aug 7, 2023 •

edited

achettyiitr Aug 7, 2023

achettyiitr Aug 7, 2023

$@fracasula$ fracasula left a comment

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

$@fracasula$ fracasula Aug 7, 2023

achettyiitr commented Aug 8, 2023

lvrach Aug 8, 2023

$@fracasula$ fracasula left a comment

chore: move warehouse handle to router #3687

chore: move warehouse handle to router #3687

Conversation

achettyiitr commented Jul 31, 2023 • edited

Description

Notion Ticket

Security

fracasula Aug 7, 2023 • edited

Choose a reason for hiding this comment

fracasula Aug 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fracasula 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

achettyiitr commented Aug 8, 2023

Choose a reason for hiding this comment

fracasula left a comment

Choose a reason for hiding this comment

achettyiitr commented Jul 31, 2023 •

edited

$@fracasula$ fracasula Aug 7, 2023 •

edited

$@fracasula$ fracasula Aug 7, 2023 •

edited

$@fracasula$ fracasula left a comment

$@fracasula$ fracasula left a comment