You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2024/12/31 10:53:34 UTC
[PR] improve broker selector utils [pinot]
gortiz opened a new pull request, #14733:
URL: https://github.com/apache/pinot/pull/14733
This PR fixes an issue on BrokerSelectorUtils when tables on distjoint brokers are used. Specifically the new test `BrokerSelectorUtilsTest.getTablesCommonBrokersListTwoTablesDifferentBrokers` fails 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] improve broker selector utils [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14733:
URL: https://github.com/apache/pinot/pull/14733#discussion_r1900632661
##########
pinot-clients/pinot-java-client/src/test/java/org/apache/pinot/client/utils/BrokerSelectorUtilsTest.java:
##########
@@ -0,0 +1,125 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.client.utils;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Set;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.Test;
+
+
+public class BrokerSelectorUtilsTest {
+
+ HashMap<String, List<String>> _brokerData = new HashMap<>();
+ @Test
+ public void getTablesCommonBrokersSetNullTables() {
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(null, _brokerData);
+ Assert.assertEquals(tableSet, Set.of());
+ }
+
+ @Test
+ public void getTablesCommonBrokersListNullTables() {
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(null, _brokerData);
+ Assert.assertNull(tableList);
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetEmptyTables() {
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of(), _brokerData);
+ Assert.assertEquals(tableSet, Set.of());
+ }
+
+ @Test
+ public void getTablesCommonBrokersListEmptyTables() {
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of(), _brokerData);
+ Assert.assertNull(tableList);
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetNotExistentTable() {
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of("notExistent"), _brokerData);
+ Assert.assertEquals(tableSet, Set.of());
+ }
+
+ @Test
+ public void getTablesCommonBrokersListNotExistentTable() {
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of("notExistent"), _brokerData);
+ Assert.assertNull(tableList);
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetOneTable() {
+ _brokerData.put("table1", List.of("broker1"));
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of("table1"), _brokerData);
+ Assert.assertEquals(tableSet.size(), 1);
+ Assert.assertTrue(tableSet.contains("broker1"));
+ }
+
+ @Test
+ public void getTablesCommonBrokersListOneTable() {
+ _brokerData.put("table1", List.of("broker1"));
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of("table1"), _brokerData);
+ Assert.assertNotNull(tableList);
+ Assert.assertEquals(tableList.size(), 1);
+ Assert.assertTrue(tableList.contains("broker1"));
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetTwoTables() {
+ _brokerData.put("table1", List.of("broker1"));
+ _brokerData.put("table2", List.of("broker1"));
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of("table1", "table2"), _brokerData);
+ Assert.assertNotNull(tableSet);
+ Assert.assertEquals(tableSet.size(), 1);
+ Assert.assertTrue(tableSet.contains("broker1"));
+ }
+
+ @Test
+ public void getTablesCommonBrokersListTwoTables() {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] improve broker selector utils [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14733:
URL: https://github.com/apache/pinot/pull/14733#discussion_r1905167309
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/utils/BrokerSelectorUtils.java:
##########
@@ -71,4 +92,26 @@ private static String getTableNameWithoutSuffix(String tableName) {
tableName.replace(ExternalViewReader.OFFLINE_SUFFIX, "").
replace(ExternalViewReader.REALTIME_SUFFIX, "");
}
+
+ /**
+ * Returns the brokers for the given table name or null if not found.
Review Comment:
Isn't that implied? I guess it is a good idea to be explicit given the list version returns null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] improve broker selector utils [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #14733:
URL: https://github.com/apache/pinot/pull/14733#issuecomment-2566343658
cc @bziobrowski a
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] improve broker selector utils [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #14733:
URL: https://github.com/apache/pinot/pull/14733#discussion_r1900541907
##########
pinot-clients/pinot-java-client/src/test/java/org/apache/pinot/client/utils/BrokerSelectorUtilsTest.java:
##########
@@ -0,0 +1,125 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.client.utils;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Set;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.Test;
+
+
+public class BrokerSelectorUtilsTest {
+
+ HashMap<String, List<String>> _brokerData = new HashMap<>();
+ @Test
+ public void getTablesCommonBrokersSetNullTables() {
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(null, _brokerData);
+ Assert.assertEquals(tableSet, Set.of());
+ }
+
+ @Test
+ public void getTablesCommonBrokersListNullTables() {
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(null, _brokerData);
+ Assert.assertNull(tableList);
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetEmptyTables() {
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of(), _brokerData);
+ Assert.assertEquals(tableSet, Set.of());
+ }
+
+ @Test
+ public void getTablesCommonBrokersListEmptyTables() {
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of(), _brokerData);
+ Assert.assertNull(tableList);
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetNotExistentTable() {
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of("notExistent"), _brokerData);
+ Assert.assertEquals(tableSet, Set.of());
+ }
+
+ @Test
+ public void getTablesCommonBrokersListNotExistentTable() {
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of("notExistent"), _brokerData);
+ Assert.assertNull(tableList);
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetOneTable() {
+ _brokerData.put("table1", List.of("broker1"));
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of("table1"), _brokerData);
+ Assert.assertEquals(tableSet.size(), 1);
+ Assert.assertTrue(tableSet.contains("broker1"));
+ }
+
+ @Test
+ public void getTablesCommonBrokersListOneTable() {
+ _brokerData.put("table1", List.of("broker1"));
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of("table1"), _brokerData);
+ Assert.assertNotNull(tableList);
+ Assert.assertEquals(tableList.size(), 1);
+ Assert.assertTrue(tableList.contains("broker1"));
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetTwoTables() {
+ _brokerData.put("table1", List.of("broker1"));
+ _brokerData.put("table2", List.of("broker1"));
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of("table1", "table2"), _brokerData);
+ Assert.assertNotNull(tableSet);
+ Assert.assertEquals(tableSet.size(), 1);
+ Assert.assertTrue(tableSet.contains("broker1"));
+ }
+
+ @Test
+ public void getTablesCommonBrokersListTwoTables() {
Review Comment:
Why does the data type of the result (List vs Set) matter ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] improve broker selector utils [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #14733:
URL: https://github.com/apache/pinot/pull/14733#discussion_r1900655573
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/utils/BrokerSelectorUtils.java:
##########
@@ -71,4 +92,26 @@ private static String getTableNameWithoutSuffix(String tableName) {
tableName.replace(ExternalViewReader.OFFLINE_SUFFIX, "").
replace(ExternalViewReader.REALTIME_SUFFIX, "");
}
+
+ /**
+ * Returns the brokers for the given table name or null if not found.
Review Comment:
nit: Returns an empty set if not found.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] improve broker selector utils [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14733:
URL: https://github.com/apache/pinot/pull/14733#issuecomment-2566372026
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14733?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`03ed9b3`)](https://app.codecov.io/gh/apache/pinot/commit/03ed9b3deb3eb21d069513a379460d393891d730?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1522 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (03ed9b3). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (03ed9b3) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|0|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14733 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 3 -2433
Lines 133233 6 -133227
Branches 20636 0 -20636
=============================================
- Hits 82274 0 -82274
+ Misses 44911 6 -44905
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14733/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14733?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] improve broker selector utils [pinot]
Posted by "bziobrowski (via GitHub)" <gi...@apache.org>.
bziobrowski commented on code in PR #14733:
URL: https://github.com/apache/pinot/pull/14733#discussion_r1900185574
##########
pinot-clients/pinot-java-client/src/test/java/org/apache/pinot/client/utils/BrokerSelectorUtilsTest.java:
##########
@@ -0,0 +1,125 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.client.utils;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Set;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.Test;
+
+
+public class BrokerSelectorUtilsTest {
+
+ HashMap<String, List<String>> _brokerData = new HashMap<>();
+ @Test
+ public void getTablesCommonBrokersSetNullTables() {
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(null, _brokerData);
+ Assert.assertEquals(tableSet, Set.of());
+ }
+
+ @Test
+ public void getTablesCommonBrokersListNullTables() {
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(null, _brokerData);
+ Assert.assertNull(tableList);
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetEmptyTables() {
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of(), _brokerData);
+ Assert.assertEquals(tableSet, Set.of());
+ }
+
+ @Test
+ public void getTablesCommonBrokersListEmptyTables() {
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of(), _brokerData);
+ Assert.assertNull(tableList);
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetNotExistentTable() {
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of("notExistent"), _brokerData);
+ Assert.assertEquals(tableSet, Set.of());
+ }
+
+ @Test
+ public void getTablesCommonBrokersListNotExistentTable() {
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of("notExistent"), _brokerData);
+ Assert.assertNull(tableList);
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetOneTable() {
+ _brokerData.put("table1", List.of("broker1"));
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of("table1"), _brokerData);
+ Assert.assertEquals(tableSet.size(), 1);
+ Assert.assertTrue(tableSet.contains("broker1"));
+ }
+
+ @Test
+ public void getTablesCommonBrokersListOneTable() {
+ _brokerData.put("table1", List.of("broker1"));
+ List<String> tableList = BrokerSelectorUtils.getTablesCommonBrokers(List.of("table1"), _brokerData);
+ Assert.assertNotNull(tableList);
+ Assert.assertEquals(tableList.size(), 1);
+ Assert.assertTrue(tableList.contains("broker1"));
+ }
+
+ @Test
+ public void getTablesCommonBrokersSetTwoTables() {
+ _brokerData.put("table1", List.of("broker1"));
+ _brokerData.put("table2", List.of("broker1"));
+ Set<String> tableSet = BrokerSelectorUtils.getTablesCommonBrokersSet(List.of("table1", "table2"), _brokerData);
+ Assert.assertNotNull(tableSet);
+ Assert.assertEquals(tableSet.size(), 1);
+ Assert.assertTrue(tableSet.contains("broker1"));
+ }
+
+ @Test
+ 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);
Review Comment:
Wouldn't a single assert be more readable ?
`Assert.assertEquals(tableList, Array.asList("broker1"))`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] improve broker selector utils [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14733:
URL: https://github.com/apache/pinot/pull/14733#discussion_r1905167309
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/utils/BrokerSelectorUtils.java:
##########
@@ -71,4 +92,26 @@ private static String getTableNameWithoutSuffix(String tableName) {
tableName.replace(ExternalViewReader.OFFLINE_SUFFIX, "").
replace(ExternalViewReader.REALTIME_SUFFIX, "");
}
+
+ /**
+ * Returns the brokers for the given table name or null if not found.
Review Comment:
~Isn't that implied? I guess it is a good idea to be explicit given the list version returns null.~
Sorry, I misread your comment. You are right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] improve broker selector utils [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #14733:
URL: https://github.com/apache/pinot/pull/14733
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org