[cleanup][broker] Fix ClusterDataImpl#clone and add test #19126
Conversation
michaeljmarshall
added
type/cleanup
michaeljmarshall
requested review from
merlimat,
lhotari,
rdhabalia and
nodece
| .migrated(true) | ||
| .migratedClusterUrl(new ClusterData.ClusterUrl("pulsar://remote", "pulsar+ssl://remote")) | ||
| .build(); | ||
|
|
There was a problem hiding this comment.
I think you should validate each field, so like:
assertEquals("JKS", originalData.getBrokerClientTlsTrustStoreType());There was a problem hiding this comment.
I will add a separate test to validate that we correctly implement equals.
There was a problem hiding this comment.
Actually, my suggested solution to test equals doesn't really make sense, especially because we use an annotation to generate the equals method.
Why do you think it is important to validate each field individually, @nodece?
There was a problem hiding this comment.
Sorry, I didn't notice the method name. You are right.
| public void verifyClone() { | ||
| ClusterDataImpl originalData = ClusterDataImpl.builder() | ||
| .serviceUrl("pulsar://test") | ||
| .serviceUrlTls("pulsar+ssl://test") |
There was a problem hiding this comment.
Maybe we could use java reflection to test new added fields.
There was a problem hiding this comment.
We shouldn't use the reflection. when the field changes, it's going to be a little difficult to maintain.
|
/pulsarbot rerun-failure-checks |
Codecov Report
@@ Coverage Diff @@
## master #19126 +/- ##
============================================
- Coverage 47.43% 47.22% -0.21%
- Complexity 9535 10694 +1159
============================================
Files 632 712 +80
Lines 59839 69645 +9806
Branches 6234 7481 +1247
============================================
+ Hits 28384 32890 +4506
- Misses 28375 33053 +4678
- Partials 3080 3702 +622
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/pulsarbot rerun-failure-checks |
Motivation
In #17962, new configurations were added to the
ClusterDataImplwithout updating theclonemethod. In #17295, new configurations were incorrectly cloned. None of these "bugs" are in a released version of pulsar.This PR fixes those issues and adds a test to verify the fixes. One gap on the test is that it won't easily test new fields unless the test is updated. I would guess we need smarter tests to make that work. I'm not aware of any solutions, but if someone has pointers, please let me know.
Modifications
ClusterDataImpl#cloneVerifying this change
A new test is added to cover the changes.
Does this pull request potentially affect one of the following parts:
This is an internal change.
Documentation
doc-not-neededMatching PR in forked repository
PR in forked repository: michaeljmarshall#10