-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
CDK: allow ConnectorStateManager stream_instance_map to take ConfiguredAirbyteStream or Stream #35000
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
b389763
to
cbb0a30
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.
Can you link the issue for the bigger refactor in this PR? Thanks!
@@ -38,7 +38,7 @@ class ConnectorStateManager: | |||
|
|||
def __init__( | |||
self, | |||
stream_instance_map: Mapping[str, AirbyteStream], | |||
stream_instance_map: Mapping[str, Union[Stream, AirbyteStream]], | |||
state: Optional[Union[List[AirbyteStateMessage], MutableMapping[str, Any]]] = None, | |||
): | |||
shared_state, per_stream_states = self._extract_from_state_message(state, stream_instance_map) |
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.
Should the signature for _extract_from_state_message
be changed as well?
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.
Yep! Updated.
ConfiguredAirbyteStream (from a catalog) instead of just AirbyteStreams
cbb0a30
to
46d6d21
Compare
Here's a the issue to refactor the ConectorStateManger. |
…edAirbyteStream or Stream (#35000)
…edAirbyteStream or Stream (airbytehq#35000)
…edAirbyteStream or Stream (airbytehq#35000)
…edAirbyteStream or Stream (airbytehq#35000)
…edAirbyteStream or Stream (airbytehq#35000)
…edAirbyteStream or Stream (#35000)
In #34540, the
ConnectorStateManager
'sstream_instance_map
was updated to takeConfiguredAirbyteStream
s instead ofStream
s, because at the point where it was being used by the file-based CDK there was a circular dependency that prevented us from havingAirbyteStream
s available.However, when revisiting #34716, which updates usages of the ConnectorStateManager, I realized that the methods called by
ConnectorStateManager
exist on bothStream
andAirbyteStream
, and so prefer to take the union of the stream types than to have conditional logic that creates a state manager only if a catalog exists, which we'd have to do to update the Stripe connector.