expose batch_size in config #129
Conversation
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
some high level comments
sentry_streams/sentry_streams/adapters/arroyo/reduce_delegate.py
Outdated
Show resolved
Hide resolved
victoria-yining-huang
force-pushed
the
vic/add_batch_size_to_config
branch
from
615bb60 to
e391e2e
Compare
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
Please see the comments in line.
I think this approach is more complex than it needs to be. If you delegate to each step the override process you will not need an app_config: Any attribute that has unspecified behavior for all the subclasses that do not allow for override.
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
Please revert the nullability of the batch_size and see the other comments in line
| def __post_init__(self) -> None: | ||
| self.ctx.register(self) | ||
|
|
||
| def override_config(self, loaded_config: Mapping[str, Any] | None) -> Any: |
There was a problem hiding this comment.
I think this function should not return anything -> None:
Also is there any difference for the function whether it is passed none or an empty Mapping? Supporting both hints that such a difference would exist.
If they mean the same thing, please disallow one by only allow Mapping[str, Any]
There was a problem hiding this comment.
done, loaded_config only allows Mapping[str, Any] now
| # TODO: Use concept of custom triggers to close window | ||
| # by either size or time | ||
| batch_size: MeasurementUnit | ||
| batch_size: Optional[MeasurementUnit] = None |
There was a problem hiding this comment.
This is not supposed to be optional. There must always be a default otherwise this introduces an implicit requirement that in some cases all regions must have an override for the batch_size.
If you require a default then we know there is a fallback value for any region that does not specify this field.
There was a problem hiding this comment.
default is mandatory now
| assert ( | ||
| self.batch_size is not None | ||
| ), f"{self.name} config must be set before windowing is accessed" |
There was a problem hiding this comment.
Not allowing None also allows you to remove this.
In general only support nullability when you need it. It will remove a lot of corner cases that you would have to test and maintain.
There was a problem hiding this comment.
removed
| "loaded_batch_size, default_batch_size, expected", | ||
| [ | ||
| pytest.param({"batch_size": 50}, 100, 50, id="Have both loaded and default values"), | ||
| pytest.param({"batch_size": 50}, None, 50, id="Only has loaded config file value"), |
There was a problem hiding this comment.
This scenario is not supposed to be allowed.
There was a problem hiding this comment.
removed. This scenario now code will error out at runtime
|
@fpacifici made default value mandatory |
We want to support
batch_sizebeing passed in either from the application code, when aBatchclass is instantiated (Batch(batch_size=n)), or when it's being passed in from the deployment_config file.There is an overriding logic: