Use NodeType enum instead of Strings#6377
Conversation
| */ | ||
| public enum NodeType | ||
| { | ||
| coordinator, |
There was a problem hiding this comment.
I think all these enum values in uppercase would be better.
There was a problem hiding this comment.
See the comment in the line 23. These names and the fact that Jackson serializes enum constants to their names as strings allows this PR to be non-breaking.
There was a problem hiding this comment.
You can customize this with a @JsonValue annotation: see LongEncodingStrategy for an example.
| DruidNodeDiscoveryProvider.NODE_TYPE_ROUTER, | ||
| DruidNodeDiscoveryProvider.NODE_TYPE_MM | ||
| ); | ||
| private static final List<NodeType> NODE_TYPES = Arrays.asList(NodeType.values()); |
There was a problem hiding this comment.
BTW not sure about this, if coordinator was skipped on purpose or by mistake. See #5099 (comment). @jon-wei
|
There is something similar to this called |
|
@jihoonson see #6384, I think this is not reasonable unless we are willing to break JSON APIs. |
|
@himanshug could you please take a look? |
|
This looks like a trivial change. But, have you tested this on a running cluster to ensure it doesn't break anything? LGTM otherwise. |
|
@himanshug we never test any changes before merging into master. Integration tests are supposed to test inter-node and ZK communications. |
|
I wish I was as confident about integration tests. In general, I think I try and test everything at least in a toy cluster when sending the PR and saying it was done from my side. Even that doesn't catch all problems but gives me as PR author some confidence. |
|
that said, this PR looks pretty trivial and it is likely that integration env will cover it but it is usually nice to test such changes. |
I think this is inefficient. A PR like this one is normally done in less than one hour, while creating a distribution, a test cluster, and testing the new code reasonably is much more work to do. The release baking process with release candidates should provide such testing "in batch" for all PRs that are included in a specific release. |
|
my two cents... Personally, I feel more confident with the work when it is tested. Code reviews catch some bugs but testing the change is always great when possible :) . |
|
Assuming this pr had +1 from @himanshug I think it is a good pr so let's get it in |
* Use NodeType enum instead of Strings * Make NodeType constants uppercase * Fix CommonCacheNotifier and NodeType/ServerType comments * Reconsidering comment * Fix import * Add a comment to CommonCacheNotifier.NODE_TYPES
8 participants
For type safety and code readability.