You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/04/09 06:21:02 UTC
[GitHub] [incubator-pinot] malbx opened a new pull request #6765: Fix the exception thrown when the table is not found
malbx opened a new pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765
## Description
Fix the exception thrown in the case that a specified table name does not exist (#6328). As opposed to returning `BROKER_RESOURCE_MISSING_ERROR`, which can be a bit misleading, the code has been modified to return `SERVER_TABLE_MISSING_ERROR_CODE`.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] malbx commented on pull request #6765: Fix the exception thrown when the table is not found
Posted by GitBox <gi...@apache.org>.
malbx commented on pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765#issuecomment-820919469
> `SERVER_TABLE_MISSING_ERROR` means the table exists, but broker routes the query to a server that does not have the table loaded. It is a system internal error.
> We need to add a new error type for non-existing table, and use `TableCache` to check if the table config exist
The new error type for non-existing table has been added.
Here is the current code that appears to determine if the table exist.
https://github.com/apache/incubator-pinot/blob/aaa28fa0446507704dfa3e4b58335a41f5c1b430/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java#L294-L300
Are you suggesting that this code should use `TableCache` to check if the table exist instead of the current approach?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6765: Fix the exception thrown when the table is not found
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765#discussion_r610955709
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java
##########
@@ -43,7 +43,7 @@
public class BrokerResponseNative implements BrokerResponse {
public static final BrokerResponseNative EMPTY_RESULT = BrokerResponseNative.empty();
public static final BrokerResponseNative NO_TABLE_RESULT =
- new BrokerResponseNative(QueryException.BROKER_RESOURCE_MISSING_ERROR);
+ new BrokerResponseNative(QueryException.SERVER_TABLE_MISSING_ERROR);
Review comment:
This may cause incompatibility with previous versions
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6765: Fix the exception thrown when the table is not found
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765#discussion_r614597860
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -294,7 +294,7 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
if ((offlineTableName == null) && (realtimeTableName == null)) {
// No table matches the request
LOGGER.info("No table matches for request {}: {}", requestId, query);
- requestStatistics.setErrorCode(QueryException.BROKER_RESOURCE_MISSING_ERROR_CODE);
+ requestStatistics.setErrorCode(QueryException.TABLE_DOES_NOT_EXIST_ERROR_CODE);
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.RESOURCE_MISSING_EXCEPTIONS, 1);
Review comment:
Yes. Currently there are 2 scenarios that can cause broker resource missing:
1. Table does not exist - should raise the table does not exist error
2. Table exists but not available in broker routing - should raise the broker resource missing 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] malbx commented on a change in pull request #6765: Fix the exception thrown when the table is not found
Posted by GitBox <gi...@apache.org>.
malbx commented on a change in pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765#discussion_r614571944
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -294,7 +294,7 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
if ((offlineTableName == null) && (realtimeTableName == null)) {
// No table matches the request
LOGGER.info("No table matches for request {}: {}", requestId, query);
- requestStatistics.setErrorCode(QueryException.BROKER_RESOURCE_MISSING_ERROR_CODE);
+ requestStatistics.setErrorCode(QueryException.TABLE_DOES_NOT_EXIST_ERROR_CODE);
Review comment:
@Jackie-Jiang Given the changes in this PR should `BrokerMeter.RESOURCE_MISSING_EXCEPTIONS` still be used 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6765: Fix the exception thrown when the table is not found
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765#discussion_r625316486
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -302,6 +302,12 @@ private BrokerResponseNative handleSQLRequest(long requestId, String query, Json
}
if ((offlineTableName == null) && (realtimeTableName == null)) {
// No table matches the request
+ if (_tableCache.getTableConfig(TableNameBuilder.REALTIME.tableNameWithType(rawTableName)) == null
+ && _tableCache.getTableConfig(TableNameBuilder.OFFLINE.tableNameWithType(rawTableName)) == null) {
+ LOGGER.info("Table not found for request {}: {}", requestId, query);
+ requestStatistics.setErrorCode(QueryException.TABLE_DOES_NOT_EXIST_ERROR_CODE);
+ return new BrokerResponseNative(QueryException.TABLE_DOES_NOT_EXIST_ERROR);
Review comment:
Suggest putting another constant response in `BrokerResponseNative` similar to `EMPTY_RESULT` and `NO_TABLE_RESULT` (we should probably rename this)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] malbx commented on a change in pull request #6765: Fix the exception thrown when the table is not found
Posted by GitBox <gi...@apache.org>.
malbx commented on a change in pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765#discussion_r614574110
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java
##########
@@ -43,7 +43,7 @@
public class BrokerResponseNative implements BrokerResponse {
public static final BrokerResponseNative EMPTY_RESULT = BrokerResponseNative.empty();
public static final BrokerResponseNative NO_TABLE_RESULT =
- new BrokerResponseNative(QueryException.BROKER_RESOURCE_MISSING_ERROR);
+ new BrokerResponseNative(QueryException.SERVER_TABLE_MISSING_ERROR);
Review comment:
Thanks for pointing this out. I will add details to the release notes section.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] malbx commented on a change in pull request #6765: Fix the exception thrown when the table is not found
Posted by GitBox <gi...@apache.org>.
malbx commented on a change in pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765#discussion_r614572360
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -294,7 +294,7 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
if ((offlineTableName == null) && (realtimeTableName == null)) {
// No table matches the request
LOGGER.info("No table matches for request {}: {}", requestId, query);
- requestStatistics.setErrorCode(QueryException.BROKER_RESOURCE_MISSING_ERROR_CODE);
+ requestStatistics.setErrorCode(QueryException.TABLE_DOES_NOT_EXIST_ERROR_CODE);
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.RESOURCE_MISSING_EXCEPTIONS, 1);
Review comment:
@Jackie-Jiang Given the changes in this PR should `BrokerMeter.RESOURCE_MISSING_EXCEPTIONS` still be used 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] malbx commented on a change in pull request #6765: Fix the exception thrown when the table is not found
Posted by GitBox <gi...@apache.org>.
malbx commented on a change in pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765#discussion_r614574110
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java
##########
@@ -43,7 +43,7 @@
public class BrokerResponseNative implements BrokerResponse {
public static final BrokerResponseNative EMPTY_RESULT = BrokerResponseNative.empty();
public static final BrokerResponseNative NO_TABLE_RESULT =
- new BrokerResponseNative(QueryException.BROKER_RESOURCE_MISSING_ERROR);
+ new BrokerResponseNative(QueryException.SERVER_TABLE_MISSING_ERROR);
Review comment:
Thanks for pointing this out. I will add details to the release notes section of this PR.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #6765: Fix the exception thrown when the table is not found
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #6765:
URL: https://github.com/apache/incubator-pinot/pull/6765
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org