Add lifecycle interface for connectors #280
Conversation
WalkthroughThe pull request introduces enhancements to the connector lifecycle management system by adding new methods and interfaces for managing lifecycle events such as start, resume, and end operations. Key modifications include the addition of a lifecycle service in protocol buffers, updates to client and server interfaces, and the implementation of lifecycle management methods across various components. These changes collectively provide a structured approach to handling different stages of a connector's operational lifecycle. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ConnectorLifeCycleService
participant Connector
Client->>ConnectorLifeCycleService: OnStart(ResourceTypeId)
ConnectorLifeCycleService->>Connector: Initialize Lifecycle
Connector-->>ConnectorLifeCycleService: Startup Complete
ConnectorLifeCycleService-->>Client: OnStartResponse
Client->>ConnectorLifeCycleService: OnResume(ResourceTypeId)
ConnectorLifeCycleService->>Connector: Resume Operations
Connector-->>ConnectorLifeCycleService: Resume Complete
ConnectorLifeCycleService-->>Client: OnResumeResponse
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
MarcusGoldschmidt
changed the title
MarcusGoldschmidt
force-pushed
the
goldschmidt/lifecycle-connector
branch
from
c400cb7 to
1b7c164
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
pkg/connectorbuilder/connectorbuilder.go (5)
86-90: Consider renaming for consistency and clarity.
The new interfaceConnectorLifeCycleis well-structured; however, consider renaming it toConnectorLifecycle(without the capital "C"), to maintain consistent naming conventions.
361-367: Nitpick: missing space in your error formatting.
When returning an error for a duplicated resource type, the substring"manager%s"will concatenate theresourceTypeIDwithout a preceding space.- return nil, fmt.Errorf("error: duplicate resource type found for life cycle manager%s", rType.Id) + return nil, fmt.Errorf("error: duplicate resource type found for life cycle manager %s", rType.Id)
943-961: Add context logging for trouble-shooting.
TheOnStartmethod callsrb.OnStart(ctx)but does not log which resource type is being started. Adding a log statement can aid debugging failures or slow startups.func (b *builderImpl) OnStart(ctx context.Context, request *v2.OnStartRequest) (*v2.OnStartResponse, error) { start := b.nowFunc() tt := tasks.OnStart resp := &v2.OnStartResponse{} + l := ctxzap.Extract(ctx) + l.Info("Connector OnStart invoked", + zap.String("resource_type_id", request.ResourceTypeId)) rb, ok := b.lifeCycleManager[request.ResourceTypeId] ... }
963-981: Ensure symmetry with other lifecycle methods.
OnResumemirrorsOnStartlogic well. Consider the same approach to logging for consistent diagnostics and auditing.
983-1002: Confirm final resource state handling.
OnEndproperly records success/failure, but consider whether additional cleanup or context logging is needed for more robust lifecycle completion. Otherwise, the approach is consistent.pkg/sync/syncer.go (3)
432-443: Leverage transition logs.
IntroducingOnStartSyncOpandOnResumeSyncOpclarifies the sync state machine. Consider adding log statements indicating these transitions for better traceability.
1693-1705: Add error context for debugging.
OnStartSyncreturns errors with minimal context. Consider logging theResourceTypeIdif you need deeper operational visibility or debugging info on failures.
1721-1733: Future usage consideration.
OnEndSyncis available but commented out in the sync flow. When implemented, ensure correct ordering in the state machine so partial syncs or timeouts do not skip finalization.proto/c1/connector/v2/resource.proto (1)
292-292: Consider future extensibility in response messages.The empty response messages (
OnStartResponse,OnResumeResponse,OnEndResponse) work for the current use case. However, consider adding an annotations field to allow for future extensibility without breaking changes.-message OnStartResponse {} +message OnStartResponse { + repeated google.protobuf.Any annotations = 1; +} -message OnResumeResponse {} +message OnResumeResponse { + repeated google.protobuf.Any annotations = 1; +} -message OnEndResponse {} +message OnEndResponse { + repeated google.protobuf.Any annotations = 1; +}Also applies to: 302-302, 312-312
pb/c1/connector/v2/resource.pb.validate.go (1)
3716-4448: Lifecycle interface validation implementation is well-structured and consistent.The validation code for the new lifecycle interface (OnStart, OnResume, OnEnd) follows a consistent pattern:
- Request messages validate ResourceTypeId and Annotations
- Response messages have appropriate empty validation
- Error types and helper methods are properly generated
- Follows established protobuf validation patterns
This provides a solid foundation for the connector lifecycle management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
internal/connector/connector.go(2 hunks)pb/c1/connector/v2/resource.pb.validate.go(1 hunks)pkg/connectorbuilder/connectorbuilder.go(4 hunks)pkg/sync/state.go(3 hunks)pkg/sync/syncer.go(5 hunks)pkg/sync/syncer_test.go(3 hunks)pkg/types/tasks/tasks.go(1 hunks)pkg/types/types.go(2 hunks)proto/c1/connector/v2/resource.proto(3 hunks)proto/c1/connector/v2/ticket.proto(0 hunks)proto/c1/connectorapi/baton/v1/baton.proto(6 hunks)
💤 Files with no reviewable changes (1)
- proto/c1/connector/v2/ticket.proto
✅ Files skipped from review due to trivial changes (1)
- proto/c1/connectorapi/baton/v1/baton.proto
🔇 Additional comments (19)
pkg/connectorbuilder/connectorbuilder.go (1)
294-294: Initialization logic is clear.
ThelifeCycleManagermap initialization looks good, ensuring that lifecycle references can be stored without nil checks. No issues spotted here.pkg/sync/syncer.go (3)
381-381: Method signature extension recognized.
ProvidingnewSyncto clarify if the sync is fresh or resumed is a good approach. Ensure other sync-related methods also reflect or handle this state accurately if needed.
610-614: Push correct lifecycle actions based on sync type.
This logic is concise and ensures the correct lifecycle operation is queued. No immediate issues.
1707-1719: Consistency across lifecycle methods.
OnResumeSyncis parallel in structure withOnStartSync, maintaining consistent flow. Good job on uniformity.pkg/types/types.go (2)
24-24: Align the naming with existing services.
AddingconnectorV2.ConnectorLifeCycleServiceServerneatly extends the interface. The naming matches the introduced lifecycle pattern.
41-41: Client pairing looks good.
ConnectorLifeCycleServiceClientpairs well with the server interface addition, enabling remote lifecycle calls.pkg/sync/state.go (1)
52-55: LGTM!The implementation of the new sync operations
OnStartSyncOpandOnResumeSyncOpis complete and consistent. All necessary methods (String(),newActionOp()) are properly updated to handle these operations.Also applies to: 95-98, 114-115
internal/connector/connector.go (1)
47-47: LGTM!The addition of
ConnectorLifeCycleServiceClientand its initialization is consistent with the existing pattern of service client management.Also applies to: 332-345
pkg/sync/syncer_test.go (3)
359-395: LGTM! Test coverage for lifecycle methods is well-implemented.The test effectively verifies the lifecycle behavior:
- Confirms that
OnStartis called during initial sync- Verifies that
OnResumeis not called during initial sync- Clear setup and assertions
399-404: LGTM! Mock connector properly integrates lifecycle client.The lifecycle client is properly added to the mock connector and initialized in
newMockConnector.Also applies to: 423-423
525-553: LGTM! Mock lifecycle client implementation is complete.The mock implementation:
- Handles all lifecycle methods (OnStart, OnResume, OnEnd)
- Properly handles nil function pointers
- Follows consistent implementation pattern
proto/c1/connector/v2/resource.proto (2)
76-81: Well-designed service interface!The
ConnectorLifeCycleServicefollows good gRPC practices with clear lifecycle stages (start, resume, end) and a simple interface design that will be easy for connector implementations to adopt.
284-290: Well-structured request messages with appropriate validation.The request messages (
OnStartRequest,OnResumeRequest,OnEndRequest) follow a consistent pattern with:
- Validated resource_type_id (1-1024 bytes)
- Extensible annotations field
This design provides good validation while maintaining flexibility through annotations.
Also applies to: 294-300, 304-310
pb/c1/connector/v2/resource.pb.validate.go (6)
3716-3859: OnStartRequest validation implementation looks good!The validation logic properly checks:
- ResourceTypeId length constraints (1-1024 bytes)
- Embedded Annotations message validation
3861-3959: OnStartResponse validation implementation looks good!The empty validation is appropriate as there are no fields requiring validation in the OnStartResponse message.
3961-4104: OnResumeRequest validation implementation looks good!The validation logic properly checks:
- ResourceTypeId length constraints (1-1024 bytes)
- Embedded Annotations message validation
The implementation maintains consistency with OnStartRequest validation.
4106-4204: OnResumeResponse validation implementation looks good!The empty validation is appropriate as there are no fields requiring validation in the OnResumeResponse message.
4206-4348: OnEndRequest validation implementation looks good!The validation logic properly checks:
- ResourceTypeId length constraints (1-1024 bytes)
- Embedded Annotations message validation
The implementation maintains consistency with OnStartRequest and OnResumeRequest validation.
4350-4448: OnEndResponse validation implementation looks good!The empty validation is appropriate as there are no fields requiring validation in the OnEndResponse message.
| OnStart | ||
| OnResume | ||
| OnEnd |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update String() method to handle new task types.
The new task types OnStart, OnResume, and OnEnd are not handled in the String() method, which will return "unknown" for these tasks. This could affect logging and debugging capabilities.
Update the String() method by adding the following cases:
func (tt TaskType) String() string {
switch tt {
+ case OnStart:
+ return "on_start"
+ case OnResume:
+ return "on_resume"
+ case OnEnd:
+ return "on_end"
default:
return "unknown"
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connectorbuilder/connectorbuilder.go (1)
943-1002: Consider refactoring to reduce duplication and improve error handling.The lifecycle methods (
OnStart,OnResume,OnEnd) share similar patterns and could benefit from refactoring:
- Extract common logic into a helper function
- Add more context to error messages
- Include logging for better observability
Consider this refactoring approach:
+func (b *builderImpl) executeLifecycleOp( + ctx context.Context, + resourceTypeId string, + op string, + tt tasks.TaskType, + fn func(context.Context) error, +) (proto.Message, error) { + l := ctxzap.Extract(ctx) + start := b.nowFunc() + resp := proto.Clone(b.emptyResponseFor(op)) + + l.Debug("executing lifecycle operation", + zap.String("operation", op), + zap.String("resource_type_id", resourceTypeId)) + + rb, ok := b.lifeCycleManager[resourceTypeId] + if !ok { + l.Debug("no lifecycle manager found, skipping", + zap.String("resource_type_id", resourceTypeId)) + b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) + return resp, nil + } + + if err := fn(ctx); err != nil { + l.Error("lifecycle operation failed", + zap.String("operation", op), + zap.String("resource_type_id", resourceTypeId), + zap.Error(err)) + b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) + return resp, fmt.Errorf("error: %s operation failed for resource type %s: %w", + op, resourceTypeId, err) + } + + l.Debug("lifecycle operation completed successfully", + zap.String("operation", op), + zap.String("resource_type_id", resourceTypeId)) + b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) + return resp, nil +} + +func (b *builderImpl) emptyResponseFor(op string) proto.Message { + switch op { + case "OnStart": + return &v2.OnStartResponse{} + case "OnResume": + return &v2.OnResumeResponse{} + case "OnEnd": + return &v2.OnEndResponse{} + default: + return nil + } +} + -func (b *builderImpl) OnStart(ctx context.Context, request *v2.OnStartRequest) (*v2.OnStartResponse, error) { - start := b.nowFunc() - tt := tasks.OnStart - resp := &v2.OnStartResponse{} - - rb, ok := b.lifeCycleManager[request.ResourceTypeId] - if ok { - err := rb.OnStart(ctx) - if err != nil { - b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) - return resp, fmt.Errorf("error: on start failed: %w", err) - } - } - - b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) - return resp, nil -} +func (b *builderImpl) OnStart(ctx context.Context, request *v2.OnStartRequest) (*v2.OnStartResponse, error) { + resp, err := b.executeLifecycleOp(ctx, request.ResourceTypeId, "OnStart", tasks.OnStart, + func(ctx context.Context) error { + return b.lifeCycleManager[request.ResourceTypeId].OnStart(ctx) + }) + return resp.(*v2.OnStartResponse), err +}Similar changes would apply to
OnResumeandOnEndmethods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/connectorbuilder/connectorbuilder.go(4 hunks)pkg/types/tasks/tasks.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/types/tasks/tasks.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (3)
pkg/connectorbuilder/connectorbuilder.go (3)
86-90: LGTM! Well-designed interface for lifecycle management.The
ConnectorLifeCycleinterface provides a clean and focused API for managing connector states through distinct lifecycle events.
97-97: LGTM! Clean implementation of lifecycle manager registration.The implementation follows good practices:
- Proper initialization during construction
- Duplicate checking for resource types
- Clean type assertion pattern for optional interface implementation
Also applies to: 294-294, 362-367
Line range hint
86-1002: Verify integration with the connector's lifecycle events.The lifecycle management implementation looks solid. To ensure proper integration, verify that these lifecycle methods are called at the appropriate times in the connector's lifecycle:
OnStart: When the connector begins its operationOnResume: When the connector resumes after a pauseOnEnd: When the connector completes its operationRun this script to verify the lifecycle method usage:
✅ Verification successful
Lifecycle methods are properly implemented and integrated
The lifecycle methods (
OnStart,OnResume,OnEnd) are correctly implemented with:
- Proper integration in the sync process via pkg/sync/syncer.go
- Complete error handling and metrics recording
- Graceful handling of cases where lifecycle manager is not implemented
- Consistent state management via FinishAction calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of lifecycle methods # Check for lifecycle method invocations echo "Checking lifecycle method invocations..." rg -A 5 "OnStart\(|OnResume\(|OnEnd\(" --type go # Check for any potential direct List() calls that should use lifecycle methods echo "Checking for potential List() calls that should use lifecycle methods..." ast-grep --pattern 'List(ctx context.Context, parentResourceID *v2.ResourceId, pToken *pagination.Token)'Length of output: 12712
Add an interface to manage the lifecycle of a connector
This replaces the workaround of method
Listwhen thepToken.Token == "" && parentResourceID == nilto know if it is the first call of the connectorSummary by CodeRabbit
New Features
Validation
Testing
Protocol Buffers