improve broker selector utils #14733
Conversation
|
cc @bziobrowski a |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14733 +/- ##
============================================
+ Coverage 61.75% 63.87% +2.12%
- Complexity 207 1607 +1400
============================================
Files 2436 2703 +267
Lines 133233 150794 +17561
Branches 20636 23295 +2659
============================================
+ Hits 82274 96317 +14043
- Misses 44911 47279 +2368
- Partials 6048 7198 +1150
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| public void getTablesCommonBrokersListTwoTables() { | ||
| _brokerData.put("table1", List.of("broker1")); | ||
| _brokerData.put("table2", List.of("broker1")); | ||
| List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of("table1", "table2"), _brokerData); |
There was a problem hiding this comment.
Wouldn't a single assert be more readable ?
Assert.assertEquals(tableList, Array.asList("broker1"))
| } | ||
|
|
||
| @Test | ||
| public void getTablesCommonBrokersListTwoTables() { |
There was a problem hiding this comment.
Why does the data type of the result (List vs Set) matter ?
There was a problem hiding this comment.
It doesn't depend on the returned data type. It is just that I'm keeping the older method for background compatibility reasons.
Implementation of the older method is not correct. Apart of being unnecessary expensive by allocating too much, the main issue is that depending on some conditions it either returns null or an empty set when there are not common brokers.
I'm just keeping the old method in case someone is using it outside this repo (I know we do), but the idea is to change or remove it in future.
...ients/pinot-java-client/src/main/java/org/apache/pinot/client/utils/BrokerSelectorUtils.java
Outdated
Show resolved
Hide resolved
This PR fixes an issue on BrokerSelectorUtils when tables on distjoint brokers are used. Specifically the new test
BrokerSelectorUtilsTest.getTablesCommonBrokersListTwoTablesDifferentBrokersfails in master because{"broker1"}was returned in that case.It also includes some cleanup in the code to reduce the complexity in terms of computation but also making it easier to read. A bunch of new test have been added as well