-
Notifications
You must be signed in to change notification settings - Fork 320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(warehouse): degraded workspace id #2627
Conversation
c015372
to
a00cdd7
Compare
…t.degraded-workspace-id
…t.degraded-workspace-id
Codecov ReportBase: 45.60% // Head: 45.44% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2627 +/- ##
==========================================
- Coverage 45.60% 45.44% -0.16%
==========================================
Files 287 289 +2
Lines 47790 47928 +138
==========================================
- Hits 21793 21783 -10
- Misses 24619 24762 +143
- Partials 1378 1383 +5
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. |
…dder-server into feat.degraded-workspace-id
@lvrach |
Yeah, I was considering this as well. I will give it a try and see how it goes. |
@achettyiitr archiver has its package now, but it has some changes to upload types to avoid circular imports. |
warehouse/multitenant/manager.go
Outdated
} | ||
|
||
// Run is a blocking function that executes manager background logic. | ||
func (m *Manager) Run(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are always returning nil. Should we remove the error it?
} | ||
|
||
m.sourceIDToWorkspaceID = make(map[string]string) | ||
m.excludeWorkspaceIDMap = make(map[string]struct{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using this anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we are using degradedWorkspaceIDs
as excludeWorkspace
. Do we still need this?
Kudos @lvrach. It was really nice going through this and learning a lot from it. |
f6afbc1
to
43fe99b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. 👍🏼
func backupRecords(args backupRecordsArgs) (backupLocation string, err error) { | ||
pkgLogger.Infof(`Starting backupRecords for uploadId: %s, sourceId: %s, destinationId: %s, tableName: %s,`, args.uploadID, args.sourceID, args.destID, args.tableName) | ||
type Archiver struct { | ||
DB *sql.DB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having a separate package for archiver and explicitly listing down the dependencies. For having a better DI, should we expose a NewArchiver
function so that the system doesn't allowing creating archiver with nil values. Wdyt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this approach for packages that are used as internal components. Let me explain,
If we were to introduce a NewArchive method, is going to need 5 args, and more will be added in the future. In this case, we could use opts fn or a separate struct for the options that still require validation.
In general, is common in Go to initialise struct like this (http.Server, for example).
If we need validation we can do it lazily as part of .Run
method for example, but personally, for internal packages, I find it overkill.
@@ -0,0 +1,228 @@ | |||
//go:build !warehouse_integration | |||
|
|||
package archive_test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing job with the test case ! Moreee tests !! 🥳 🎉
feat(warehouse): added support for bigquery custom partition for workspaceIDs (#2679) chore: by default enable max concurrent gw request limit. (#2648) doc: create SECURITY.md (#2656) chore: use bugsnag in router and processor goroutines (#2686) test: using arm64 compatible images if necessary (#2670) chore: regulation worker avoid panic in case of timeout (#2657) feat(warehouse): degraded workspace id (#2627) Release-As: 1.4.0
feat(warehouse): added support for bigquery custom partition for workspaceIDs (#2679) chore: by default enable max concurrent gw request limit. (#2648) doc: create SECURITY.md (#2656) chore: use bugsnag in router and processor goroutines (#2686) test: using arm64 compatible images if necessary (#2670) chore: regulation worker avoid panic in case of timeout (#2657) feat(warehouse): degraded workspace id (#2627) Release-As: 1.4.0
Description
Feature
Degradation is a common concept in rudder-stack infrastructure, where a service runs in a limited way allowing some operations to happen safely. In the warehouse domain, degraded mode means:
Workspace degradation is a feature that enables applying the degradation mode for specific workspaceIDs.
Motivation
Implementation
A new component is introduced,
multitenant.Manager
is responsible for:Places that have been affected:
http.StatusServiceUnavailable
, if a workspace id is degraded.Archive refactoring
To properly test the changes of the archiver:
limitation and out-of-scope
Degraded workspaces are statically defined. A service restart will be required every time we change them. The manager is design is a way to easily support dynamic workspace ids in the future.
The manager is not properly injected in all the cases. To use proper DI for the
manager
significant refactoring will be required.async framework is not in the scope of this PR.
Notion Ticket
https://www.notion.so/rudderstacks/workspace-degraded-mode-5f231fa3899b4a79bf230d2ee12e85ca
Security