IWF-357: Add internal channel TypeStore #70
Conversation
| import inspect | ||
| import time | ||
| import unittest | ||
|
|
There was a problem hiding this comment.
Test added to make sure the currently existing issue is fixed. Issue description by @longquanzheng
The MVP solution works but not ideal – it just blend/mix the prefix and non-prefix channel names without differentiation. This could cause some confusion/unexpected behavior.
For example:
- User can define a channel name “ABC” (not by prefix) and try to publish with name “ABCD” will also be allowed – but it should be disallowed. Because “ABC” is not by prefix.
| INTERNAL_CHANNEL = 1 | ||
| # TODO: extend to other types | ||
| # DATA_ATTRIBUTE = 2 | ||
| # SIGNAL_CHANNEL = 3 |
There was a problem hiding this comment.
JavaSDK allows prefixing SignalChannels and DataAttributes. Leaving this for future use
lwolczynski
requested review from
mixydubs,
samuel27m,
bowersj27,
ktrops and
stevo89519
| self._name_to_type_store = dict() | ||
| self._prefix_to_type_store = dict() | ||
|
|
||
| def is_valid_name_or_prefix(self, name: str) -> bool: |
There was a problem hiding this comment.
I changed it to be used now. It was not used before, good catch
iwf/type_store.py
Outdated
| t = self._do_get_type(name) | ||
|
|
||
| if t is None: | ||
| raise ValueError(f"{self._class_type} not registered: {name}") |
There was a problem hiding this comment.
Should this be a WorkflowDefinitionError since the type has not been registered in the store?
There was a problem hiding this comment.
I was thinking about it as well. I think we have two options here:
- Change the exception type from
ValueErrortoWorkflowDefinitionError - Let
get_typereturnNoneand let the caller do the exception handling
I noticed that publish_to_internal_channel in communication.py and from_idl_command_results in command_results.py will never get to their exception handling if it's done in type_store.py. Thoughts?
There was a problem hiding this comment.
You mentioned it here as well
There was a problem hiding this comment.
Since this error is fatal, I think we should still raise something here, instead of returning None. In the caller we should wrap the get_type call in a try-except block. Maybe here we should create our own specific exception like NotRegisteredError and in the caller we except it and raise a WorkflowDefinitionError. We want to chain the exceptions, so that the first exception is preserved in the stack trace.
example here in type_store.py in get_type():
if t is None:
raise NotRegisteredError(f"{self._class_type} not registered: {name}")
example in the caller (i.e. communication.py):
try:
registered_type = self._internal_channel_type_store.get_type(channel_name)
except NotRegisteredError as exception:
raise WorkflowDefinitionError(f"InternalChannel channel_name is not defined {channel_name}") from exception
There was a problem hiding this comment.
Nice! I think that's a great idea. Thanks for the help! Added it.
iwf/type_store.py
Outdated
|
|
||
| def add_internal_channel_def(self, obj: CommunicationMethod): | ||
| if self._class_type != Type.INTERNAL_CHANNEL: | ||
| raise WorkflowDefinitionError( |
There was a problem hiding this comment.
I might have this switched, but if a workflow is adding an internal channel definition and we are filtering them in the SDK in line 94 of registry.py by checking CommunicationMethodType.InternalChannel then it's not a WorkflowDefinitionError, but an SDK error. The user didn't define something incorrectly, but the SDK is filtering incorrectly, so maybe this should be a ValueError .
There was a problem hiding this comment.
That's a good point 👍 changed it to ValueError
Closes #41