You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Nice, well-scoped feature that rides on Storm's native component-config mechanism.
Bolt config is applied once per incoming stream, not once per bolt. The block applying memory/CPU/tasks/config runs on every iteration of the streams loop, so a bolt with N inputs calls validateFields() + addConfigurations() N times on the same declarer. It's idempotent, so not a bug, and it matches the existing pattern for setMemoryLoad/setCPULoad/setNumTasks (just redundant=
Validation asymmetry. buildConfig() applies the topology-wide config: with no ConfigValidation, while this PR validates per-component config. Fail-fast is the better behavior, but the asymmetry (an invalid-typed value passes topology-wide yet fails per-component) may surprise users. Consider validating the topology config too in a follow-up, or noting the difference.
README clarity. "Known Storm configuration keys are validated …" is accurate but could read as blanket validation. Suggest adding: "Unknown/custom keys are not validated and are passed through verbatim; validation runs client-side at build time."
Nit: new HashMap<String, Object>() in VertexDef → diamond <> to match the codebase.
Thank you for the reviews! I have just pushed a new round of commits to address your feedback.
@rzo1
Redundant Loop Execution: I refactored the logic using the new functional switch pattern to ensure the component declarer is defined only once during the first stream, eliminating the redundant calls.
Validation asymmetry: Updated the logic to validate the topology-wide configuration as well, ensuring a consistent fail-fast behaviour across both levels.
Documentation and nit: fixed.
@reiabreu
Regarding the topology-level override test: I did some further investigation. Because the merged configuration is evaluated at runtime, a full verification would technically require an integration test. However, since FluxBuilder acts as a wrapper for TopologyBuilder, and TopologyBuilder's overriding behaviour is already covered under TopologyIntegrationTest#testComponentSpecificConfig, I opted for a lighter approach to avoid test duplication.
I have added a unit test in TCKTest.java to verify that merging the topology and component configurations via Utils.merge() executes without conflicts. If you still feel a full integration test is necessary here, please let me know, and I'll be happy to add it!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
Add per-component configuration in the flux topology def grammar.
How was the change tested
Fix Flux support per-component configuration in
BoltDefandSpoutDef#8710