[GitHub] [incubator-pinot] snleee commented on a change in pull request #4615: Update Getting Started documentation.
snleee commented on a change in pull request #4615: Update Getting Started
documentation.
URL: https://github.com/apache/incubator-pinot/pull/4615#discussion_r325477904
##
File path: docs/getting_started.rst
##
@@ -114,7 +114,29 @@ Suppose we have a transcript in CSV format containing
students' basic info and t
| 202| Nick | Young |Male | Physics |3.6|
+++---+---+---+---+
-Firstly in order to set up a table, we need to specify the schema of this
transcript.
+We will not include a header line in this CSV file:
+
+.. code-block:: none
+
+ 200,Lucy,Smith,Female,Maths,3.8
+ 200,Lucy,Smith,Female,English,3.5
+ 201,Bob,King,Male,Maths,3.2
+ 202,Nick,Young,Male,Physics,3.6
+
+Instead of using a header line, we will use a separate CSV config JSON file:
+
+.. code-block:: none
+
+ {
+"header":"studentID,firstName,lastName,gender,subject,score",
+"fileFormat":"CSV"
+ }
+
+We can create a working directory called `getting-started` on Desktop, and
create two additional directories within `getting-started` called `data`
Review comment:
Let's be more specific.
```
$ cd $WORKING_DIR
$ mkdir getting-started
$ mkdir getting-started/data
$ mkdir getting started/config
```
and add raw data & store in `$WORKING_DIR
/getting-started/data/transcript.csv`, csv config at
`$WORKING_DIR/config/csv-record-reader-config.json` and so on.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[incubator-pinot] branch new_segment_creation updated (2930748 -> 4e4b7be)
This is an automated email from the ASF dual-hosted git repository. xiangfu pushed a change to branch new_segment_creation in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git. from 2930748 Adding CreateSegmentCommandV2 command add 4e4b7be Address comments No new revisions were added by this update. Summary of changes: .../pinot/tools/admin/command/CreateSegmentCommand.java | 3 ++- .../pinot/tools/admin/command/CreateSegmentCommandV2.java | 11 +-- 2 files changed, 11 insertions(+), 3 deletions(-) - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.
jamesyfshao commented on a change in pull request #4608: Unit tests and bug
fixes for DeleteTable rest API for controller.
URL: https://github.com/apache/incubator-pinot/pull/4608#discussion_r325444901
##
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
##
@@ -254,14 +254,20 @@ public SuccessResponse deleteTable(
List tablesDeleted = new LinkedList<>();
try {
- if (tableType != TableType.REALTIME &&
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+ if (tableType != TableType.REALTIME &&
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)
+ && _pinotHelixResourceManager.hasOfflineTable(tableName)) {
_pinotHelixResourceManager.deleteOfflineTable(tableName);
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
}
- if (tableType != TableType.OFFLINE &&
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+ if (tableType != TableType.OFFLINE &&
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)
+ && _pinotHelixResourceManager.hasRealtimeTable(tableName)) {
_pinotHelixResourceManager.deleteRealtimeTable(tableName);
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
}
+ if (tablesDeleted.size() == 0) {
Review comment:
I think it would be better to think of this change as an improvement to the
pinot admin API feedback. set aside the debates over the correct way to respond
to a DELETE on a resource that is not presented, This change provides the
caller of this method more information by providing feedback if such method
doesn't exist. If the users desire the old behavior, he/she can just check if
the method returns non-500 return codes. But now our users are also able to
check if the table is really in pinot system. Given how this change improves
the existing pinot system behavior, I think we should accept this diff after
fixing any further minor issues @Jackie-Jiang
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on issue #4602: First pass of GROUP BY with ORDER BY support
codecov-io edited a comment on issue #4602: First pass of GROUP BY with ORDER BY support URL: https://github.com/apache/incubator-pinot/pull/4602#issuecomment-530129911 # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4602?src=pr=h1) Report > Merging [#4602](https://codecov.io/gh/apache/incubator-pinot/pull/4602?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/dbcbd2eae2430050c003de86c605a7ed8f0e1b9e?src=pr=desc) will **increase** coverage by `0.2%`. > The diff coverage is `81.91%`. [](https://codecov.io/gh/apache/incubator-pinot/pull/4602?src=pr=tree) ```diff @@ Coverage Diff @@ ## master#4602 +/- ## === + Coverage 64.23% 64.43% +0.2% Complexity 32 32 === Files 1072 1074 +2 Lines 5570256078+376 Branches 8131 8198 +67 === + Hits 3577836133+355 + Misses1727617264 -12 - Partials 2648 2681 +33 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/4602?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...org/apache/pinot/common/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `42.3% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: | | [...e/query/aggregation/groupby/GroupKeyGenerator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9ncm91cGJ5L0dyb3VwS2V5R2VuZXJhdG9yLmphdmE=) | `100% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: | | [...ava/org/apache/pinot/pql/parsers/Pql2Compiler.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9QcWwyQ29tcGlsZXIuamF2YQ==) | `64.88% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: | | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `87.5% <0%> (+37.5%)` | `0 <0> (ø)` | :arrow_down: | | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `85.71% <0%> (-14.29%)` | `0 <0> (ø)` | | | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `75% <0%> (-2.5%)` | `0 <0> (ø)` | | | [...ion/groupby/AggregationGroupByTrimmingService.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9ncm91cGJ5L0FnZ3JlZ2F0aW9uR3JvdXBCeVRyaW1taW5nU2VydmljZS5qYXZh) | `94.44% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: | | [...t/common/response/broker/BrokerResponseNative.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzcG9uc2UvYnJva2VyL0Jyb2tlclJlc3BvbnNlTmF0aXZlLmphdmE=) | `91.57% <100%> (+0.27%)` | `0 <0> (ø)` | :arrow_down: | | [...tion/groupby/DictionaryBasedGroupKeyGenerator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9ncm91cGJ5L0RpY3Rpb25hcnlCYXNlZEdyb3VwS2V5R2VuZXJhdG9yLmphdmE=) | `96.84% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: | | [...org/apache/pinot/core/data/table/IndexedTable.java](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0luZGV4ZWRUYWJsZS5qYXZh) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: | | ... and [42 more](https://codecov.io/gh/apache/incubator-pinot/pull/4602/diff?src=pr=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4602?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.
jamesyfshao commented on a change in pull request #4608: Unit tests and bug
fixes for DeleteTable rest API for controller.
URL: https://github.com/apache/incubator-pinot/pull/4608#discussion_r325443739
##
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
##
@@ -254,20 +254,41 @@ public SuccessResponse deleteTable(
List tablesDeleted = new LinkedList<>();
try {
- if (tableType != TableType.REALTIME &&
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+ if (toDeleteOfflineTable(tableName, tableType) &&
_pinotHelixResourceManager
+ .hasOfflineTable(tableName)) {
_pinotHelixResourceManager.deleteOfflineTable(tableName);
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
}
- if (tableType != TableType.OFFLINE &&
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+ if (toDeleteRealtimeTable(tableName, tableType) &&
_pinotHelixResourceManager
+ .hasRealtimeTable(tableName)) {
_pinotHelixResourceManager.deleteRealtimeTable(tableName);
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
}
+ if (tablesDeleted.size() == 0) {
+throw new ControllerApplicationException(LOGGER, "Table '" + tableName
+ "' does not exist",
+Response.Status.NOT_FOUND);
+ }
return new SuccessResponse("Tables: " + tablesDeleted + " deleted");
} catch (Exception e) {
- throw new ControllerApplicationException(LOGGER, e.getMessage(),
Response.Status.INTERNAL_SERVER_ERROR, e);
+ throw new ControllerApplicationException(LOGGER, e.getMessage(),
+ Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+ // Return true if tableType is NOT offline (i.e., null or realtime) and
table name does not end
+ // with _offline suffix.
+ private boolean toDeleteRealtimeTable(String tableName, TableType tableType)
{
Review comment:
I feel this two method can use better naming, for example,
ensureTableNameMatchType(..) and potentially merge two method into one. We can
probably even remove the comments if the method naming is improved
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325439739 ## File path: pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java ## @@ -41,20 +43,25 @@ private static ControllerLeaderLocator _instance = null; public static final Logger LOGGER = LoggerFactory.getLogger(ControllerLeaderLocator.class); + // Minimum millis which must elapse between consecutive invalidation of cache + private static final long MILLIS_BETWEEN_INVALIDATE = 30_000L; + private final HelixManager _helixManager; - // Co-ordinates of the last known controller leader. - private Pair _controllerLeaderHostPort = null; + // Co-ordinates of the last known controller leader for each of the lead-controller every partitions, + // with partition number being the key and controller hostname and port pair being the value. If the lead + // controller resource is disabled in the configuration then this map contains helix cluster leader co-ordinates + // for all partitions of leadControllerResource. + private final Map> _cachedControllerLeaderMap; - // Indicates whether cached controller leader value is invalid - private volatile boolean _cachedControllerLeaderInvalid = true; + // Indicates whether cached controller leader(s) value is(are) invalid. + private volatile boolean _cachedControllerLeaderValid = false; // Time in millis when cache invalidate was last set private volatile long _lastCacheInvalidateMillis = 0; Review comment: `_lastCacheInvalidationTimeMs` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325440855
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ Set partitionNames =
leadControllerResourceExternalView.getPartitionSet();
+ if (partitionNames.isEmpty()) {
+LOGGER.warn("The partition set in the external view of {} is empty.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ if (partitionNames.size() !=
Helix.NUMBER_OF_PARTITIONS_IN_LEAD_CONTROLLER_RESOURCE) {
+LOGGER.warn("The partition size of
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325440772
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ Set partitionNames =
leadControllerResourceExternalView.getPartitionSet();
+ if (partitionNames.isEmpty()) {
Review comment:
(nit) This check can be merged into the next one
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325437581 ## File path: pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java ## @@ -41,20 +43,25 @@ private static ControllerLeaderLocator _instance = null; public static final Logger LOGGER = LoggerFactory.getLogger(ControllerLeaderLocator.class); + // Minimum millis which must elapse between consecutive invalidation of cache + private static final long MILLIS_BETWEEN_INVALIDATE = 30_000L; Review comment: Rename to `MIN_INVALIDATE_INTERVAL_MS`? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325439471
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -229,22 +251,26 @@ private boolean isLeadControllerResourceEnabled() {
* Thus the frequency limiting is done to guard against frequent cache
refreshes, in cases where we might be getting too many NOT_SENT responses due
to some other errors.
*/
public synchronized void invalidateCachedControllerLeader() {
-long now = System.currentTimeMillis();
+long now = getCurrentTimeMS();
long millisSinceLastInvalidate = now - _lastCacheInvalidateMillis;
if (millisSinceLastInvalidate < MILLIS_BETWEEN_INVALIDATE) {
LOGGER.info(
"Millis since last controller cache value invalidate {} is less than
allowed frequency {}. Skipping invalidate.",
millisSinceLastInvalidate, MILLIS_BETWEEN_INVALIDATE);
} else {
LOGGER.info("Invalidating cached controller leader value");
- _cachedControllerLeaderInvalid = true;
+ _cachedControllerLeaderValid = false;
_lastCacheInvalidateMillis = now;
}
}
+ protected long getCurrentTimeMS() {
Review comment:
Rename to `getCurrentTimeMs()`. Also, is this visible for testing?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325441081
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ Set partitionNames =
leadControllerResourceExternalView.getPartitionSet();
+ if (partitionNames.isEmpty()) {
+LOGGER.warn("The partition set in the external view of {} is empty.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ if (partitionNames.size() !=
Helix.NUMBER_OF_PARTITIONS_IN_LEAD_CONTROLLER_RESOURCE) {
+LOGGER.warn("The partition size of
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325440450
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
Review comment:
To me the warnings for line 128, 133, 137 should be logged as error
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325438318 ## File path: pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java ## @@ -41,20 +43,25 @@ private static ControllerLeaderLocator _instance = null; public static final Logger LOGGER = LoggerFactory.getLogger(ControllerLeaderLocator.class); + // Minimum millis which must elapse between consecutive invalidation of cache + private static final long MILLIS_BETWEEN_INVALIDATE = 30_000L; + private final HelixManager _helixManager; - // Co-ordinates of the last known controller leader. - private Pair _controllerLeaderHostPort = null; + // Co-ordinates of the last known controller leader for each of the lead-controller every partitions, + // with partition number being the key and controller hostname and port pair being the value. If the lead + // controller resource is disabled in the configuration then this map contains helix cluster leader co-ordinates + // for all partitions of leadControllerResource. + private final Map> _cachedControllerLeaderMap; - // Indicates whether cached controller leader value is invalid - private volatile boolean _cachedControllerLeaderInvalid = true; + // Indicates whether cached controller leader(s) value is(are) invalid. Review comment: lol let's revert this comment change, and reversed boolean logic? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325440281
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
Review comment:
We don't need this extra refreshSucceeded. Just set
_cachedControllerLeaderValid to true as the last statement
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on issue #4615: Update Getting Started documentation.
codecov-io edited a comment on issue #4615: Update Getting Started documentation. URL: https://github.com/apache/incubator-pinot/pull/4615#issuecomment-531921434 # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4615?src=pr=h1) Report > Merging [#4615](https://codecov.io/gh/apache/incubator-pinot/pull/4615?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/fe7fff6288464aaf430ab850741000481fa704fc?src=pr=desc) will **increase** coverage by `0.04%`. > The diff coverage is `n/a`. [](https://codecov.io/gh/apache/incubator-pinot/pull/4615?src=pr=tree) ```diff @@ Coverage Diff @@ ## master#4615 +/- ## + Coverage 64.33% 64.38% +0.04% + Complexity 94 32 -62 Files 1072 1072 Lines 5570255702 Branches 8131 8131 + Hits 3583635862 +26 + Misses1721617197 -19 + Partials 2650 2643 -7 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/4615?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...er/validation/BrokerResourceValidationManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL0Jyb2tlclJlc291cmNlVmFsaWRhdGlvbk1hbmFnZXIuamF2YQ==) | `25% <0%> (-56.25%)` | `0% <0%> (ø)` | | | [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `66.07% <0%> (-12.5%)` | `0% <0%> (ø)` | | | [.../apache/pinot/common/config/RealtimeTagConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL1JlYWx0aW1lVGFnQ29uZmlnLmphdmE=) | `93.33% <0%> (-6.67%)` | `0% <0%> (ø)` | | | [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `69.56% <0%> (-6.53%)` | `0% <0%> (ø)` | | | [...elix/core/relocation/RealtimeSegmentRelocator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlbG9jYXRpb24vUmVhbHRpbWVTZWdtZW50UmVsb2NhdG9yLmphdmE=) | `35% <0%> (-5%)` | `0% <0%> (ø)` | | | [...troller/helix/core/retention/RetentionManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JldGVudGlvbi9SZXRlbnRpb25NYW5hZ2VyLmphdmE=) | `75% <0%> (-4.17%)` | `0% <0%> (ø)` | | | [...pinot/core/operator/docidsets/OrBlockDocIdSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZHNldHMvT3JCbG9ja0RvY0lkU2V0LmphdmE=) | `84.9% <0%> (-3.78%)` | `0% <0%> (ø)` | | | [...e/operator/dociditerators/MVScanDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9NVlNjYW5Eb2NJZEl0ZXJhdG9yLmphdmE=) | `57.14% <0%> (-3.18%)` | `0% <0%> (ø)` | | | [.../org/apache/pinot/transport/netty/NettyServer.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtdHJhbnNwb3J0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC90cmFuc3BvcnQvbmV0dHkvTmV0dHlTZXJ2ZXIuamF2YQ==) | `80.8% <0%> (-3.04%)` | `0% <0%> (ø)` | | | [.../controller/helix/core/SegmentDeletionManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1NlZ21lbnREZWxldGlvbk1hbmFnZXIuamF2YQ==) | `76.22% <0%> (-1.64%)` | `0% <0%> (ø)` | | | ... and [20 more](https://codecov.io/gh/apache/incubator-pinot/pull/4615/diff?src=pr=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4615?src=pr=continue). > **Legend** - [Click here to learn
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4615: Update Getting Started documentation.
snleee commented on a change in pull request #4615: Update Getting Started documentation. URL: https://github.com/apache/incubator-pinot/pull/4615#discussion_r325418567 ## File path: docs/getting_started.rst ## @@ -114,7 +114,25 @@ Suppose we have a transcript in CSV format containing students' basic info and t | 202| Nick | Young |Male | Physics |3.6| +++---+---+---+---+ -Firstly in order to set up a table, we need to specify the schema of this transcript. +We will not include a header line in this CSV file: + +.. code-block:: none + + 200,Lucy,Smith,Female,Maths,3.8 + 200,Lucy,Smith,Female,English,3.5 + 201,Bob,King,Male,Maths,3.2 + 202,Nick,Young,Male,Physics,3.6 + +Instead of using a header line, we will use a separate CSV config JSON file: + +.. code-block:: none Review comment: It's a bit not clear on where we store these code blocks. I think that we should mention that we store the original data file @ `$WORKING_DIR/data/` and csv config @ `$WORKING_DIR/config/csv-record-reader-config.json` Same for all code blocks representing the files :) 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on issue #4553: Refactor ControllerLeaderLocator
codecov-io edited a comment on issue #4553: Refactor ControllerLeaderLocator URL: https://github.com/apache/incubator-pinot/pull/4553#issuecomment-524232323 # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4553?src=pr=h1) Report > Merging [#4553](https://codecov.io/gh/apache/incubator-pinot/pull/4553?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/fe7fff6288464aaf430ab850741000481fa704fc?src=pr=desc) will **increase** coverage by `0.04%`. > The diff coverage is `91.3%`. [](https://codecov.io/gh/apache/incubator-pinot/pull/4553?src=pr=tree) ```diff @@ Coverage Diff @@ ## master#4553 +/- ## + Coverage 64.33% 64.38% +0.04% + Complexity 94 32 -62 Files 1072 1072 Lines 5570255721 +19 Branches 8131 8137 +6 + Hits 3583635875 +39 + Misses1721617204 -12 + Partials 2650 2642 -8 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/4553?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [.../pinot/common/utils/helix/LeadControllerUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvTGVhZENvbnRyb2xsZXJVdGlscy5qYXZh) | `83.33% <100%> (+3.33%)` | `0 <0> (ø)` | :arrow_down: | | [...pinot/server/realtime/ControllerLeaderLocator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VydmVyL3JlYWx0aW1lL0NvbnRyb2xsZXJMZWFkZXJMb2NhdG9yLmphdmE=) | `91.74% <91.17%> (-3.87%)` | `0 <0> (ø)` | | | [...elix/core/relocation/RealtimeSegmentRelocator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlbG9jYXRpb24vUmVhbHRpbWVTZWdtZW50UmVsb2NhdG9yLmphdmE=) | `25% <0%> (-15%)` | `0% <0%> (ø)` | | | [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `69.64% <0%> (-8.93%)` | `0% <0%> (ø)` | | | [...pache/pinot/core/util/SortedRangeIntersection.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL1NvcnRlZFJhbmdlSW50ZXJzZWN0aW9uLmphdmE=) | `83.82% <0%> (-7.36%)` | `0% <0%> (ø)` | | | [.../apache/pinot/common/config/RealtimeTagConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL1JlYWx0aW1lVGFnQ29uZmlnLmphdmE=) | `93.33% <0%> (-6.67%)` | `0% <0%> (ø)` | | | [...ller/validation/OfflineSegmentIntervalChecker.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL09mZmxpbmVTZWdtZW50SW50ZXJ2YWxDaGVja2VyLmphdmE=) | `71.79% <0%> (-1.29%)` | `0% <0%> (ø)` | | | [...va/org/apache/pinot/common/data/TimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZGF0YS9UaW1lRmllbGRTcGVjLmphdmE=) | `92.59% <0%> (-1.24%)` | `0% <0%> (ø)` | | | [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `53.89% <0%> (-1.2%)` | `0% <0%> (ø)` | | | [...pby/NoDictionarySingleColumnGroupKeyGenerator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9ncm91cGJ5L05vRGljdGlvbmFyeVNpbmdsZUNvbHVtbkdyb3VwS2V5R2VuZXJhdG9yLmphdmE=) | `87.5% <0%> (-0.97%)` | `0% <0%> (ø)` | | | ... and [20 more](https://codecov.io/gh/apache/incubator-pinot/pull/4553/diff?src=pr=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4553?src=pr=continue). > **Legend** - [Click here to learn
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r323414056 ## File path: pinot-core/src/main/java/org/apache/pinot/core/indexsegment/mutable/MutableSegmentImpl.java ## @@ -21,14 +21,8 @@ import com.google.common.base.Preconditions; import it.unimi.dsi.fastutil.ints.IntArrays; import java.io.IOException; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; Review comment: I think that it's better to import required class only instead of importing all classes in util? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r323415168
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/index/column/ColumnIndexContainer.java
##
@@ -44,5 +45,15 @@
*/
Dictionary getDictionary();
+ /**
+ *
+ * @return Get the bloom filter for the column if it exists else {@code
null}
+ */
BloomFilterReader getBloomFilter();
+
+ /**
+ *
+ * @return Get the bloom filter for the column if it exists else {@code
null}
Review comment:
Change the comment
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325409640
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##
@@ -254,6 +261,13 @@ private boolean
createDictionaryForColumn(ColumnIndexCreationInfo info, SegmentG
@Override
public void indexRow(GenericRow row) {
+
Review comment:
remove line
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325361949
##
File path:
pinot-core/src/test/java/org/apache/pinot/core/realtime/impl/presence/RealtimePresenceVectorReaderWriterTest.java
##
@@ -0,0 +1,42 @@
+/**
+ * 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.core.realtime.impl.presence;
+
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+public class RealtimePresenceVectorReaderWriterTest {
+private static RealtimePresenceVectorReaderWriter readerWriter = null;
+
+@BeforeClass
+public void setup() {
+readerWriter = new RealtimePresenceVectorReaderWriter();
+}
+
+@Test
+public void testRealtimePresenceVectorReaderWriter() {
+for (int i=0;i<100;i++) {
+readerWriter.setNull(i);
+}
+for (int i=0;i<100;i++) {
Review comment:
same here
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325409528
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/realtime/impl/presence/RealtimePresenceVectorReaderWriter.java
##
@@ -0,0 +1,41 @@
+/**
+ * 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.core.realtime.impl.presence;
+
+import org.apache.pinot.core.segment.index.readers.PresenceVectorReader;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+/**
+ * Defines a real-time presence vector to be used in realtime ingestion.
+ */
+public class RealtimePresenceVectorReaderWriter implements
PresenceVectorReader {
+private final MutableRoaringBitmap _nullBitmap;
Review comment:
We use 2 spaces. Can you reformat?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325413616
##
File path:
pinot-core/src/test/java/org/apache/pinot/core/realtime/impl/presence/RealtimePresenceVectorReaderWriterTest.java
##
@@ -0,0 +1,42 @@
+/**
+ * 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.core.realtime.impl.presence;
+
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+public class RealtimePresenceVectorReaderWriterTest {
Review comment:
2 spaces
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325387059
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/PresenceVectorReaderImpl.java
##
@@ -0,0 +1,42 @@
+/**
+ * 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.core.segment.index.readers;
+
+import com.google.common.base.Joiner;
+
+import java.io.IOException;
+import java.util.Random;
+import org.apache.pinot.core.segment.memory.PinotDataBuffer;
+import org.roaringbitmap.RoaringBitmap;
+import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
+
+
+public class PresenceVectorReaderImpl implements PresenceVectorReader {
+
+ ImmutableRoaringBitmap _nullBitmap;
+
+ public PresenceVectorReaderImpl(PinotDataBuffer presenceVectorBuffer) throws
IOException {
+_nullBitmap = new
ImmutableRoaringBitmap(presenceVectorBuffer.toDirectByteBuffer(0,
(int)presenceVectorBuffer.size()));
Review comment:
`(int) presenceVectorBuffer.size()`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325360431
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/docidsets/BitmapDocIdSet.java
##
@@ -33,7 +36,8 @@
public BitmapDocIdSet(ImmutableRoaringBitmap[] bitmaps, int startDocId, int
endDocId, boolean exclusive) {
int numBitmaps = bitmaps.length;
if (numBitmaps > 1) {
- MutableRoaringBitmap orBitmap = MutableRoaringBitmap.or(bitmaps);
+ Iterator iterator = Arrays.asList(bitmaps).iterator();
+ MutableRoaringBitmap orBitmap = MutableRoaringBitmap.or(iterator,
startDocId, endDocId + 1);
Review comment:
Is this change about a bug fix or API change?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r323414056 ## File path: pinot-core/src/main/java/org/apache/pinot/core/indexsegment/mutable/MutableSegmentImpl.java ## @@ -21,14 +21,8 @@ import com.google.common.base.Preconditions; import it.unimi.dsi.fastutil.ints.IntArrays; import java.io.IOException; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; Review comment: I think that it's better to import required class only instead of importing all classes in util? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325413513
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/PresenceVectorReader.java
##
@@ -0,0 +1,35 @@
+/**
+ * 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.core.segment.index.readers;
+
+/**
+ * Reader interface to read from an underlying Presence vector. This is
+ * primarily used to check if a particular column value corresponding to
+ * a document ID is null or not.
+ */
+public interface PresenceVectorReader {
+
+/**
+ * Check if the given docId is present in the corresponding column
Review comment:
2 spaces
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325361903
##
File path:
pinot-core/src/test/java/org/apache/pinot/core/realtime/impl/presence/RealtimePresenceVectorReaderWriterTest.java
##
@@ -0,0 +1,42 @@
+/**
+ * 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.core.realtime.impl.presence;
+
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+public class RealtimePresenceVectorReaderWriterTest {
+private static RealtimePresenceVectorReaderWriter readerWriter = null;
+
+@BeforeClass
+public void setup() {
+readerWriter = new RealtimePresenceVectorReaderWriter();
+}
+
+@Test
+public void testRealtimePresenceVectorReaderWriter() {
+for (int i=0;i<100;i++) {
Review comment:
format for loop (`for (int i = 0; i < 100; i++_)`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #4585: Presence vector
snleee commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325413576
##
File path:
pinot-core/src/test/java/org/apache/pinot/core/indexsegment/mutable/MutableSegmentImplPresenceVectorTest.java
##
@@ -0,0 +1,104 @@
+/**
+ * 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.core.indexsegment.mutable;
+
+import org.apache.pinot.common.data.FieldSpec;
+import org.apache.pinot.common.data.Schema;
+import org.apache.pinot.core.data.GenericRow;
+import org.apache.pinot.core.data.readers.JSONRecordReader;
+import org.apache.pinot.core.data.readers.RecordReader;
+import org.apache.pinot.core.data.recordtransformer.CompositeTransformer;
+import org.apache.pinot.core.segment.index.data.source.ColumnDataSource;
+import org.apache.pinot.core.segment.index.readers.PresenceVectorReader;
+import org.roaringbitmap.RoaringBitmap;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+
+import static
org.apache.pinot.common.utils.CommonConstants.Segment.NULL_FIELDS;
+
+public class MutableSegmentImplPresenceVectorTest {
Review comment:
2spaces
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.
jackjlli commented on a change in pull request #4608: Unit tests and bug fixes
for DeleteTable rest API for controller.
URL: https://github.com/apache/incubator-pinot/pull/4608#discussion_r325409807
##
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java
##
@@ -273,6 +275,76 @@ public void rebalanceTableWithoutSegments()
Assert.assertEquals(rebalanceResult.getStatus(),
RebalanceResult.Status.NO_OP);
}
+ @Test
+ public void testDeleteTable() throws IOException {
+// Case 1: Create a REALTIME table and delete it directly w/o using query
param.
+TableConfig realtimeTableConfig =
_realtimeBuilder.setTableName("table0").build();
+String creationResponse =
+sendPostRequest(_createTableUrl,
realtimeTableConfig.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table0_REALTIME
succesfully added\"}");
+
+// Delete realtime table using REALTIME suffix.
+String deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "testRealtimeTable_REALTIME"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[testRealtimeTable_REALTIME] deleted\"}");
+
+// Case 2: Create an offline table and delete it directly w/o using query
param.
+TableConfig offlineTableConfig =
_offlineBuilder.setTableName("table0").build();
+creationResponse =
+sendPostRequest(_createTableUrl,
offlineTableConfig.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table0_OFFLINE
succesfully added\"}");
+
+// Delete offline table using OFFLINE suffix.
+deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "table0_OFFLINE"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[table0_OFFLINE] deleted\"}");
+
+// Case 3: Create REALTIME and OFFLINE tables and delete both of them.
+TableConfig rtConfig1 = _realtimeBuilder.setTableName("table1").build();
+creationResponse =
+sendPostRequest(_createTableUrl, rtConfig1.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table1_REALTIME
succesfully added\"}");
+
+TableConfig offlineConfig1 =
_offlineBuilder.setTableName("table1").build();
+creationResponse =
+sendPostRequest(_createTableUrl, offlineConfig1.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table1_OFFLINE
succesfully added\"}");
+
+deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "table1"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[table1_OFFLINE, table1_REALTIME] deleted\"}");
+
+
+// Case 4: Create REALTIME and OFFLINE tables and delete the
realtime/offline table using query params.
+TableConfig rtConfig2 = _realtimeBuilder.setTableName("table2").build();
+creationResponse =
+sendPostRequest(_createTableUrl, rtConfig2.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table2_REALTIME
succesfully added\"}");
+
+TableConfig offlineConfig2 =
_offlineBuilder.setTableName("table2").build();
+creationResponse =
+sendPostRequest(_createTableUrl, offlineConfig2.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table2_OFFLINE
succesfully added\"}");
+
+deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "table2?type=realtime"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[table2_REALTIME] deleted\"}");
+
+deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "table2?type=offline"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[table2_OFFLINE] deleted\"}");
+
+// Case 5: Delete a non-existent table and expect a bad request expection.
+try {
+ deleteResponse = sendDeleteRequest(
+ StringUtil.join("/", this._controllerBaseApiUrl, "tables",
"no_such_table_OFFLINE"));
Review comment:
Add `Assert.fail()` right after this line.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[incubator-pinot] branch proxy deleted (was ddb8c5f)
This is an automated email from the ASF dual-hosted git repository. jenniferdai pushed a change to branch proxy in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git. was ddb8c5f Adding a constructor to create the HttpClient with a proxy The revisions that were on this branch are still contained in other references; therefore, this change does not discard any commits from the repository. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jenniferdai closed pull request #4582: Allow creation of HttpClient with a proxy
jenniferdai closed pull request #4582: Allow creation of HttpClient with a proxy URL: https://github.com/apache/incubator-pinot/pull/4582 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on issue #4583: Support default value for Byte column
codecov-io edited a comment on issue #4583: Support default value for Byte column URL: https://github.com/apache/incubator-pinot/pull/4583#issuecomment-529687169 # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4583?src=pr=h1) Report > Merging [#4583](https://codecov.io/gh/apache/incubator-pinot/pull/4583?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/a1c5af7034d20070bd8f1c87c9ac2774d8044f1c?src=pr=desc) will **increase** coverage by `0.1%`. > The diff coverage is `91.66%`. [](https://codecov.io/gh/apache/incubator-pinot/pull/4583?src=pr=tree) ```diff @@ Coverage Diff @@ ## master#4583 +/- ## === + Coverage 64.42% 64.52% +0.1% Complexity 32 32 === Files 1072 1072 Lines 5563855709 +71 Branches 8112 8131 +19 === + Hits 3584235948+106 + Misses1714217115 -27 + Partials 2654 2646 -8 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/4583?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...loader/defaultcolumn/BaseDefaultColumnHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L2luZGV4L2xvYWRlci9kZWZhdWx0Y29sdW1uL0Jhc2VEZWZhdWx0Q29sdW1uSGFuZGxlci5qYXZh) | `92.05% <100%> (+0.16%)` | `0 <0> (ø)` | :arrow_down: | | [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `78.04% <100%> (+0.79%)` | `0 <0> (ø)` | :arrow_down: | | [...gment/index/readers/ImmutableDictionaryReader.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L2luZGV4L3JlYWRlcnMvSW1tdXRhYmxlRGljdGlvbmFyeVJlYWRlci5qYXZh) | `90.55% <50%> (+0.07%)` | `0 <0> (ø)` | :arrow_down: | | [...ore/query/scheduler/resources/ResourceManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvcmVzb3VyY2VzL1Jlc291cmNlTWFuYWdlci5qYXZh) | `85.71% <0%> (-10.72%)` | `0% <0%> (ø)` | | | [...pache/pinot/core/util/SortedRangeIntersection.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL1NvcnRlZFJhbmdlSW50ZXJzZWN0aW9uLmphdmE=) | `83.82% <0%> (-7.36%)` | `0% <0%> (ø)` | | | [...edicate/BaseDictionaryBasedPredicateEvaluator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL0Jhc2VEaWN0aW9uYXJ5QmFzZWRQcmVkaWNhdGVFdmFsdWF0b3IuamF2YQ==) | `60.71% <0%> (-4.68%)` | `0% <0%> (ø)` | | | [...der/HighLevelConsumerBasedRoutingTableBuilder.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9idWlsZGVyL0hpZ2hMZXZlbENvbnN1bWVyQmFzZWRSb3V0aW5nVGFibGVCdWlsZGVyLmphdmE=) | `90.9% <0%> (-3.04%)` | `0% <0%> (ø)` | | | [.../pinot/core/query/scheduler/PriorityScheduler.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvUHJpb3JpdHlTY2hlZHVsZXIuamF2YQ==) | `78.08% <0%> (-2.74%)` | `0% <0%> (ø)` | | | [...e/operator/dociditerators/SVScanDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9TVlNjYW5Eb2NJZEl0ZXJhdG9yLmphdmE=) | `63.21% <0%> (-2.26%)` | `0% <0%> (ø)` | | | [...lter/predicate/RangePredicateEvaluatorFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL1JhbmdlUHJlZGljYXRlRXZhbHVhdG9yRmFjdG9yeS5qYXZh) | `88.46% <0%> (-1.54%)` | `0% <0%> (ø)` | | | ... and [29 more](https://codecov.io/gh/apache/incubator-pinot/pull/4583/diff?src=pr=tree-more) | | -- [Continue to review full report at
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4583: Support default value for Byte column
Jackie-Jiang commented on a change in pull request #4583: Support default value for Byte column URL: https://github.com/apache/incubator-pinot/pull/4583#discussion_r325378744 ## File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java ## @@ -303,6 +304,10 @@ protected void createColumnV1Indices(String column) dictionaryElementSize = StringUtil.encodeUtf8(stringDefaultValue).length; Review comment: @kishoreg We always encode String using UTF-8. As long as the files are generated with Pinot library, the String will be encoded with UTF-8. Record reader should be able to decode other format if necessary. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on issue #4553: Refactor ControllerLeaderLocator
mcvsubbu commented on issue #4553: Refactor ControllerLeaderLocator URL: https://github.com/apache/incubator-pinot/pull/4553#issuecomment-532395787 Please hold off merging until we are out of code yellow, thanks 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #4553: Refactor ControllerLeaderLocator
mcvsubbu commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325378284
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -181,6 +181,7 @@ private void
refreshControllerLeaderMapFromHelixClusterLeader() {
try {
Pair helixClusterLeader = getHelixClusterLeader();
if (helixClusterLeader == null) {
+LOGGER.error("Failed to refresh the controller leader map.");
Review comment:
you are right, there is only one failure mode, so let us leave it as it is.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4553: Refactor ControllerLeaderLocator
jackjlli commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325376173
##
File path:
pinot-core/src/test/java/org/apache/pinot/server/realtime/ControllerLeaderLocatorTest.java
##
@@ -68,45 +69,48 @@ public void testInvalidateCachedControllerLeader() {
// Create Controller Leader Locator
FakeControllerLeaderLocator.create(helixManager);
-ControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
+FakeControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
// check values at startup
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(), 0);
// very first invalidate
+long currentTimeMs = 31_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
long lastCacheInvalidateMillis =
controllerLeaderLocator.getLastCacheInvalidateMillis();
Assert.assertTrue(lastCacheInvalidateMillis > 0);
+Assert.assertEquals(lastCacheInvalidateMillis, currentTimeMs);
// invalidate within {@link
ControllerLeaderLocator::getMillisBetweenInvalidate()} millis
// values should remain unchanged
-lastCacheInvalidateMillis = System.currentTimeMillis();
-
controllerLeaderLocator.setLastCacheInvalidateMillis(lastCacheInvalidateMillis);
+currentTimeMs = 32_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
// getControllerLeader, which validates the cache
controllerLeaderLocator.getControllerLeader(testTable);
-
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
// invalidate within {@link
ControllerLeaderLocator::getMillisBetweenInvalidate()} millis
// values should remain unchanged
-lastCacheInvalidateMillis = System.currentTimeMillis();
-
controllerLeaderLocator.setLastCacheInvalidateMillis(lastCacheInvalidateMillis);
+currentTimeMs = 33_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderInvalid());
-
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
+Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderValid());
+Assert.assertTrue(controllerLeaderLocator.getLastCacheInvalidateMillis() <
currentTimeMs);
Review comment:
Removed.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #4553: Refactor ControllerLeaderLocator
mcvsubbu commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325371893
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -177,16 +177,20 @@ private void
refreshControllerLeaderMapFromLeadControllerResource() {
* Updates lead controller pairs from Helix cluster leader.
*/
private void refreshControllerLeaderMapFromHelixClusterLeader() {
-Pair helixClusterLeader = getHelixClusterLeader();
-if (helixClusterLeader == null) {
- _cachedControllerLeaderValid = false;
- return;
-}
-for (int i = 0; i <
Helix.NUMBER_OF_PARTITIONS_IN_LEAD_CONTROLLER_RESOURCE; i++) {
- _cachedControllerLeaderMap.put(i, helixClusterLeader);
+boolean refreshSucceeded = false;
+try {
+ Pair helixClusterLeader = getHelixClusterLeader();
+ if (helixClusterLeader == null) {
+return;
+ }
+ for (int i = 0; i <
Helix.NUMBER_OF_PARTITIONS_IN_LEAD_CONTROLLER_RESOURCE; i++) {
+_cachedControllerLeaderMap.put(i, helixClusterLeader);
+ }
+ refreshSucceeded = true;
+ LOGGER.info("Refreshed controller leader map successfully.");
+} finally {
+ _cachedControllerLeaderValid = refreshSucceeded;
Review comment:
Please add a log if the refresh failed for any reason.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #4553: Refactor ControllerLeaderLocator
mcvsubbu commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325371496
##
File path:
pinot-core/src/test/java/org/apache/pinot/server/realtime/ControllerLeaderLocatorTest.java
##
@@ -68,45 +69,48 @@ public void testInvalidateCachedControllerLeader() {
// Create Controller Leader Locator
FakeControllerLeaderLocator.create(helixManager);
-ControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
+FakeControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
// check values at startup
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(), 0);
// very first invalidate
+long currentTimeMs = 31_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
long lastCacheInvalidateMillis =
controllerLeaderLocator.getLastCacheInvalidateMillis();
Assert.assertTrue(lastCacheInvalidateMillis > 0);
+Assert.assertEquals(lastCacheInvalidateMillis, currentTimeMs);
// invalidate within {@link
ControllerLeaderLocator::getMillisBetweenInvalidate()} millis
// values should remain unchanged
-lastCacheInvalidateMillis = System.currentTimeMillis();
-
controllerLeaderLocator.setLastCacheInvalidateMillis(lastCacheInvalidateMillis);
+currentTimeMs = 32_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
// getControllerLeader, which validates the cache
controllerLeaderLocator.getControllerLeader(testTable);
-
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
// invalidate within {@link
ControllerLeaderLocator::getMillisBetweenInvalidate()} millis
// values should remain unchanged
-lastCacheInvalidateMillis = System.currentTimeMillis();
-
controllerLeaderLocator.setLastCacheInvalidateMillis(lastCacheInvalidateMillis);
+currentTimeMs = 33_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderInvalid());
-
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
+Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderValid());
+Assert.assertTrue(controllerLeaderLocator.getLastCacheInvalidateMillis() <
currentTimeMs);
Review comment:
You don't need line 106 now :)
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4583: Support default value for Byte column
jackjlli commented on a change in pull request #4583: Support default value for
Byte column
URL: https://github.com/apache/incubator-pinot/pull/4583#discussion_r325371131
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##
@@ -574,8 +575,14 @@ public static void
addColumnMetadataInfo(PropertiesConfiguration properties, Str
if (defaultNullValue == null) {
defaultNullValue = fieldSpec.getDefaultNullValue();
}
-properties.setProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
DEFAULT_NULL_VALUE),
-String.valueOf(defaultNullValue));
+if (fieldSpec.getDataType() == FieldSpec.DataType.BYTES &&
defaultNullValue instanceof byte[]) {
Review comment:
Removed.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4583: Support default value for Byte column
jackjlli commented on a change in pull request #4583: Support default value for
Byte column
URL: https://github.com/apache/incubator-pinot/pull/4583#discussion_r325371081
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##
@@ -574,8 +575,14 @@ public static void
addColumnMetadataInfo(PropertiesConfiguration properties, Str
if (defaultNullValue == null) {
Review comment:
Correct, we have default null value for all kinds of types in FieldSpec.
Removed.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[incubator-pinot] branch byte-default-value updated (96af561 -> 99e963d)
This is an automated email from the ASF dual-hosted git repository. jlli pushed a change to branch byte-default-value in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git. from 96af561 Add tests for Tdigest and HLL byte columns add 99e963d Address PR comments No new revisions were added by this update. Summary of changes: .../pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] chenboat edited a comment on issue #4618: Make Kafka offset out of range as an transient exception.
chenboat edited a comment on issue #4618: Make Kafka offset out of range as an transient exception. URL: https://github.com/apache/incubator-pinot/pull/4618#issuecomment-532385271 > Your first point is wrong. The validation manager will put in newer offsets that are valid before restarting. No manual fix is needed. Thanks for pointing out this. The main point of this diff remains though: OFFSET_OUT_OF_RANGE in our tests are largely transient error and can be overcome by retries. The validation manager only kicks in after a set period of 10 mins as we observed -- which is too long for strict SLA for realtime ingestion. The new error code reassignment does not change the validation manager behaviors -- it just give more chances for retries. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4553: Refactor ControllerLeaderLocator
jackjlli commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325367348
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,122 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ Set partitionNames =
leadControllerResourceExternalView.getPartitionSet();
+ if (partitionNames.isEmpty()) {
+LOGGER.warn("The partition set in the external view of {} is empty.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ if (partitionNames.size() !=
Helix.NUMBER_OF_PARTITIONS_IN_LEAD_CONTROLLER_RESOURCE) {
+LOGGER.warn("The partition size of {} is
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4553: Refactor ControllerLeaderLocator
jackjlli commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325367412
##
File path:
pinot-core/src/test/java/org/apache/pinot/server/realtime/ControllerLeaderLocatorTest.java
##
@@ -68,45 +69,48 @@ public void testInvalidateCachedControllerLeader() {
// Create Controller Leader Locator
FakeControllerLeaderLocator.create(helixManager);
-ControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
+FakeControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
// check values at startup
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(), 0);
// very first invalidate
+long currentTimeMs = 31_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
long lastCacheInvalidateMillis =
controllerLeaderLocator.getLastCacheInvalidateMillis();
Assert.assertTrue(lastCacheInvalidateMillis > 0);
+Assert.assertEquals(lastCacheInvalidateMillis, currentTimeMs);
// invalidate within {@link
ControllerLeaderLocator::getMillisBetweenInvalidate()} millis
// values should remain unchanged
-lastCacheInvalidateMillis = System.currentTimeMillis();
-
controllerLeaderLocator.setLastCacheInvalidateMillis(lastCacheInvalidateMillis);
+currentTimeMs = 32_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
// getControllerLeader, which validates the cache
controllerLeaderLocator.getControllerLeader(testTable);
-
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
// invalidate within {@link
ControllerLeaderLocator::getMillisBetweenInvalidate()} millis
// values should remain unchanged
-lastCacheInvalidateMillis = System.currentTimeMillis();
-
controllerLeaderLocator.setLastCacheInvalidateMillis(lastCacheInvalidateMillis);
+currentTimeMs = 33_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderInvalid());
-
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
+Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderValid());
+Assert.assertTrue(controllerLeaderLocator.getLastCacheInvalidateMillis() <
currentTimeMs);
Review comment:
Added.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on issue #4618: Make Kafka offset out of range as an transient exception.
chenboat commented on issue #4618: Make Kafka offset out of range as an transient exception. URL: https://github.com/apache/incubator-pinot/pull/4618#issuecomment-532385271 > Your first point is wrong. The validation manager will put in newer offsets that are valid before restarting. No manual fix is needed. Thanks for pointing out this. The main point of this diff remains though: the validation manager only kicks in after a set period of 10 mins as we observed -- which is too long for strict SLA for realtime ingestion. The new error code reassignment does not change the validation manager behaviors -- it just give more chances for retries. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #4553: Refactor ControllerLeaderLocator
mcvsubbu commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325359295
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,122 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ Set partitionNames =
leadControllerResourceExternalView.getPartitionSet();
+ if (partitionNames.isEmpty()) {
+LOGGER.warn("The partition set in the external view of {} is empty.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ if (partitionNames.size() !=
Helix.NUMBER_OF_PARTITIONS_IN_LEAD_CONTROLLER_RESOURCE) {
+LOGGER.warn("The partition size of {} is
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #4553: Refactor ControllerLeaderLocator
mcvsubbu commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325360390
##
File path:
pinot-core/src/test/java/org/apache/pinot/server/realtime/ControllerLeaderLocatorTest.java
##
@@ -68,45 +69,48 @@ public void testInvalidateCachedControllerLeader() {
// Create Controller Leader Locator
FakeControllerLeaderLocator.create(helixManager);
-ControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
+FakeControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
// check values at startup
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(), 0);
// very first invalidate
+long currentTimeMs = 31_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
long lastCacheInvalidateMillis =
controllerLeaderLocator.getLastCacheInvalidateMillis();
Assert.assertTrue(lastCacheInvalidateMillis > 0);
+Assert.assertEquals(lastCacheInvalidateMillis, currentTimeMs);
// invalidate within {@link
ControllerLeaderLocator::getMillisBetweenInvalidate()} millis
// values should remain unchanged
-lastCacheInvalidateMillis = System.currentTimeMillis();
-
controllerLeaderLocator.setLastCacheInvalidateMillis(lastCacheInvalidateMillis);
+currentTimeMs = 32_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
// getControllerLeader, which validates the cache
controllerLeaderLocator.getControllerLeader(testTable);
-
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
// invalidate within {@link
ControllerLeaderLocator::getMillisBetweenInvalidate()} millis
// values should remain unchanged
-lastCacheInvalidateMillis = System.currentTimeMillis();
-
controllerLeaderLocator.setLastCacheInvalidateMillis(lastCacheInvalidateMillis);
+currentTimeMs = 33_000L;
+controllerLeaderLocator.setCurrentTimeMs(currentTimeMs);
controllerLeaderLocator.invalidateCachedControllerLeader();
-
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderInvalid());
-
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(),
lastCacheInvalidateMillis);
+Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderValid());
+Assert.assertTrue(controllerLeaderLocator.getLastCacheInvalidateMillis() <
currentTimeMs);
Review comment:
you can check if it is equal to 32_000
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on issue #4618: Make Kafka offset out of range as an transient exception.
mcvsubbu commented on issue #4618: Make Kafka offset out of range as an transient exception. URL: https://github.com/apache/incubator-pinot/pull/4618#issuecomment-532377804 Your first point is wrong. The validation manager will put in newer offsets that are valid before restarting. No manual fix is needed. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4583: Support default value for Byte column
Jackie-Jiang commented on a change in pull request #4583: Support default value
for Byte column
URL: https://github.com/apache/incubator-pinot/pull/4583#discussion_r325355324
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##
@@ -574,8 +575,14 @@ public static void
addColumnMetadataInfo(PropertiesConfiguration properties, Str
if (defaultNullValue == null) {
defaultNullValue = fieldSpec.getDefaultNullValue();
}
-properties.setProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
DEFAULT_NULL_VALUE),
-String.valueOf(defaultNullValue));
+if (fieldSpec.getDataType() == FieldSpec.DataType.BYTES &&
defaultNullValue instanceof byte[]) {
Review comment:
We just need the second check
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4583: Support default value for Byte column
Jackie-Jiang commented on a change in pull request #4583: Support default value
for Byte column
URL: https://github.com/apache/incubator-pinot/pull/4583#discussion_r325355088
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##
@@ -574,8 +575,14 @@ public static void
addColumnMetadataInfo(PropertiesConfiguration properties, Str
if (defaultNullValue == null) {
Review comment:
I don't think defaultNullValue can ever be 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support
npawar commented on a change in pull request #4602: First pass of GROUP BY with
ORDER BY support
URL: https://github.com/apache/incubator-pinot/pull/4602#discussion_r325348903
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/table/ConcurrentIndexedTable.java
##
@@ -101,31 +108,51 @@ public int size() {
@Override
public Iterator iterator() {
+if (_sort && CollectionUtils.isNotEmpty(_orderBy)) {
+ List sortedList = new ArrayList<>(_lookupMap.values());
+ sortedList.sort(_orderByComparator);
+ return sortedList.iterator();
+}
return _lookupMap.values().iterator();
}
private void resize(int trimToSize) {
if (_lookupMap.size() > trimToSize) {
- // make min heap of elements to evict
- int heapSize = _lookupMap.size() - trimToSize;
- PriorityQueue minHeap = new PriorityQueue<>(heapSize,
_minHeapComparator);
-
- for (Record record : _lookupMap.values()) {
-if (minHeap.size() < heapSize) {
- minHeap.offer(record);
-} else {
- Record peek = minHeap.peek();
- if (minHeap.comparator().compare(record, peek) < 0) {
-minHeap.poll();
+ if (CollectionUtils.isNotEmpty(_orderBy)) {
+// drop bottom
+
+// make min heap of elements to evict
+int heapSize = _lookupMap.size() - trimToSize;
+PriorityQueue minHeap = new PriorityQueue<>(heapSize,
_minHeapComparator);
+
+for (Record record : _lookupMap.values()) {
+ if (minHeap.size() < heapSize) {
minHeap.offer(record);
+ } else {
+Record peek = minHeap.peek();
+if (minHeap.comparator().compare(record, peek) < 0) {
+ minHeap.poll();
+ minHeap.offer(record);
+}
}
}
- }
- for (Record evictRecord : minHeap) {
-_lookupMap.remove(evictRecord.getKey());
+for (Record evictRecord : minHeap) {
+ _lookupMap.remove(evictRecord.getKey());
+}
+ } else {
+// drop randomly
+
+int numRecordsToDrop = _lookupMap.size() - trimToSize;
Review comment:
Handled this. Please take a look. @mayankshriv FYI
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4553: Refactor ControllerLeaderLocator
jackjlli commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325333416
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,122 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ Set partitionNames =
leadControllerResourceExternalView.getPartitionSet();
+ if (partitionNames.isEmpty()) {
+LOGGER.warn("The partition set in the external view of {} is empty.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ if (partitionNames.size() !=
Helix.NUMBER_OF_PARTITIONS_IN_LEAD_CONTROLLER_RESOURCE) {
+LOGGER.warn("The partition size of {}
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4553: Refactor ControllerLeaderLocator
jackjlli commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r32546
##
File path:
pinot-core/src/test/java/org/apache/pinot/server/realtime/ControllerLeaderLocatorTest.java
##
@@ -68,45 +69,48 @@ public void testInvalidateCachedControllerLeader() {
// Create Controller Leader Locator
FakeControllerLeaderLocator.create(helixManager);
-ControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
+FakeControllerLeaderLocator controllerLeaderLocator =
FakeControllerLeaderLocator.getInstance();
// check values at startup
-
Assert.assertTrue(controllerLeaderLocator.isCachedControllerLeaderInvalid());
+
Assert.assertFalse(controllerLeaderLocator.isCachedControllerLeaderValid());
Assert.assertEquals(controllerLeaderLocator.getLastCacheInvalidateMillis(), 0);
// very first invalidate
+long currentTimeMs = System.currentTimeMillis();
Review comment:
`System.currentTimeMillis();` has been removed from the test.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #4553: Refactor ControllerLeaderLocator
jackjlli commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r32577
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -229,22 +246,27 @@ private boolean isLeadControllerResourceEnabled() {
* Thus the frequency limiting is done to guard against frequent cache
refreshes, in cases where we might be getting too many NOT_SENT responses due
to some other errors.
*/
public synchronized void invalidateCachedControllerLeader() {
-long now = System.currentTimeMillis();
+long now = getCurrentTimeMS();
long millisSinceLastInvalidate = now - _lastCacheInvalidateMillis;
if (millisSinceLastInvalidate < MILLIS_BETWEEN_INVALIDATE) {
LOGGER.info(
"Millis since last controller cache value invalidate {} is less than
allowed frequency {}. Skipping invalidate.",
millisSinceLastInvalidate, MILLIS_BETWEEN_INVALIDATE);
} else {
LOGGER.info("Invalidating cached controller leader value");
- _cachedControllerLeaderInvalid = true;
+ _cachedControllerLeaderValid = false;
_lastCacheInvalidateMillis = now;
}
}
@VisibleForTesting
Review comment:
Done.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[incubator-pinot] branch master updated (8d593f3 -> c7b647a)
This is an automated email from the ASF dual-hosted git repository. akshayrai09 pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git. from 8d593f3 [TE] Bug fix to initialize the configDAO (#4616) add c7b647a [TE][Composite-Alert] Entity Anomaly Merger (#4609) No new revisions were added by this update. Summary of changes: .../thirdeye/detection/DefaultDataProvider.java| 33 ++--- .../thirdeye/detection/algorithm/MergeWrapper.java | 75 ++-- .../thirdeye/detection/spi/model/AnomalySlice.java | 52 ++-- .../wrapper/BaselineFillingMergeWrapper.java | 19 ++- .../wrapper/ChildKeepingMergeWrapper.java | 2 +- .../wrapper/EntityAnomalyMergeWrapper.java | 77 .../thirdeye/detection/wrapper/GrouperWrapper.java | 2 +- .../yaml/translator/DetectionConfigTranslator.java | 18 ++- .../content/templates/MetricAnomaliesContent.java | 2 +- .../pinot/thirdeye/detection/DataProviderTest.java | 2 +- .../wrapper/EntityAnomalyMergeWrapperTest.java | 136 + .../compositePipelineTranslatorTestResult-1.json | 93 +++--- 12 files changed, 405 insertions(+), 106 deletions(-) create mode 100644 thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/EntityAnomalyMergeWrapper.java create mode 100644 thirdeye/thirdeye-pinot/src/test/java/org/apache/pinot/thirdeye/detection/wrapper/EntityAnomalyMergeWrapperTest.java - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] akshayrai merged pull request #4609: [TE][Composite-Alert] Entity Anomaly Merger
akshayrai merged pull request #4609: [TE][Composite-Alert] Entity Anomaly Merger URL: https://github.com/apache/incubator-pinot/pull/4609 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on issue #4618: Make Kafka offset out of range as an transient exception.
chenboat commented on issue #4618: Make Kafka offset out of range as an transient exception. URL: https://github.com/apache/incubator-pinot/pull/4618#issuecomment-532348196 By "a segment created before kafka rolled over data", do you mean the case where the consumer asks for data with offset starting with A but all brokers have their min offsets > A. In such scenario, I think this change will fare as well as the current codes and in some cases work better. Here is the comparison: 1. Current code: It will stop ingestion immediately because the OUT_OF_RANGE is a permanent error. The Validation manager will try to restart the ingestion but that will fail again if the offset problem still persists. We would need to do manual fix. 2. New code: the transient error status means the retry will kick in: the consumer thread will try a few more brokers to check their data range. If all of them have the same issue, the behavior will be the same as the current code. If any broker still have the data, the ingestion will continue without waiting for the validation manager. Our experience so far is that putting OUT_OF_RANGE as transient_error with more retries eliminated all ingestion stopping problems due to faulty brokers. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support
npawar commented on a change in pull request #4602: First pass of GROUP BY with
ORDER BY support
URL: https://github.com/apache/incubator-pinot/pull/4602#discussion_r325291420
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/table/ConcurrentIndexedTable.java
##
@@ -101,31 +108,51 @@ public int size() {
@Override
public Iterator iterator() {
+if (_sort && CollectionUtils.isNotEmpty(_orderBy)) {
+ List sortedList = new ArrayList<>(_lookupMap.values());
+ sortedList.sort(_orderByComparator);
+ return sortedList.iterator();
+}
return _lookupMap.values().iterator();
}
private void resize(int trimToSize) {
if (_lookupMap.size() > trimToSize) {
- // make min heap of elements to evict
- int heapSize = _lookupMap.size() - trimToSize;
- PriorityQueue minHeap = new PriorityQueue<>(heapSize,
_minHeapComparator);
-
- for (Record record : _lookupMap.values()) {
-if (minHeap.size() < heapSize) {
- minHeap.offer(record);
-} else {
- Record peek = minHeap.peek();
- if (minHeap.comparator().compare(record, peek) < 0) {
-minHeap.poll();
+ if (CollectionUtils.isNotEmpty(_orderBy)) {
+// drop bottom
+
+// make min heap of elements to evict
+int heapSize = _lookupMap.size() - trimToSize;
+PriorityQueue minHeap = new PriorityQueue<>(heapSize,
_minHeapComparator);
+
+for (Record record : _lookupMap.values()) {
+ if (minHeap.size() < heapSize) {
minHeap.offer(record);
+ } else {
+Record peek = minHeap.peek();
+if (minHeap.comparator().compare(record, peek) < 0) {
+ minHeap.poll();
+ minHeap.offer(record);
+}
}
}
- }
- for (Record evictRecord : minHeap) {
-_lookupMap.remove(evictRecord.getKey());
+for (Record evictRecord : minHeap) {
+ _lookupMap.remove(evictRecord.getKey());
+}
+ } else {
+// drop randomly
+
+int numRecordsToDrop = _lookupMap.size() - trimToSize;
Review comment:
Ahh yes.. In that case, we either need the table to support `get(key)`, or
we need to make the upsert check `if (isNewKey && isCapacityFull && !isOrderBy)`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on issue #4617: Adding CreateSegmentCommandV2 command
kishoreg commented on issue #4617: Adding CreateSegmentCommandV2 command URL: https://github.com/apache/incubator-pinot/pull/4617#issuecomment-532276771 Yes. Which custom config are useful? Can we put them as part of tableConfig. I am trying to see if we can move towards 3 main sections to setup a table Schema TableConfig IngestionConfig Batch/stream 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on issue #4617: Adding CreateSegmentCommandV2 command
mcvsubbu commented on issue #4617: Adding CreateSegmentCommandV2 command URL: https://github.com/apache/incubator-pinot/pull/4617#issuecomment-532272767 In that case, let us add a print to the existing one to direct the user to this new command, and mention that it is deprecated. Over time, we can remove the older one. I have not looked through fully, but we must add a hook to pick up custom config, and naming schemes. If one is not doing anything special like I mentioned above, the V2 command should work, 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on issue #4617: Adding CreateSegmentCommandV2 command
kishoreg commented on issue #4617: Adding CreateSegmentCommandV2 command URL: https://github.com/apache/incubator-pinot/pull/4617#issuecomment-532259749 Existing command is too complicated and needs segment generation config. I would like to simplify this. Touching existing will break compatibility, so it’s better add a new one and deprecate the existing one. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] chenboat opened a new pull request #4618: Make Kafka offset out of range as an transient exception.
chenboat opened a new pull request #4618: Make Kafka offset out of range as an transient exception. URL: https://github.com/apache/incubator-pinot/pull/4618 During LLC migration in Uber, we often found LLC consumer often stopped consumption because it is assigned a bad Kafka broker (and resulted in OFFSET_OUT_OF_RANGE error). The current way to assigning OFFSET_OUT_OF_RANGE as permanent error causing the realtime consumption to stop -- until a background fixer thread to revive the consumption in ~10mins. In a company with large number of Kafka brokers, faulty brokers occur pretty frequently. A stop of realtime ingestion for 10 mins is not acceptable due to high SLA. We rather mark the error as a transient one and let the retry to kick in to select a different brokers. It works well after the error code reassignments. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
