[Feature] Add a second version implementation of get table schema and partition API. #41938
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
satanson
previously approved these changes
Mar 5, 2024
plotor
force-pushed
the
feature-table-schema-api-v2-zhenchao-20240227
branch
from
5fcb372 to
204eea9
Compare
plotor
force-pushed
the
feature-table-schema-api-v2-zhenchao-20240227
branch
4 times, most recently
from
3ecf130 to
cccd226
Compare
plotor
force-pushed
the
feature-table-schema-api-v2-zhenchao-20240227
branch
from
cccd226 to
e91bd5b
Compare
plotor
changed the title
kevincai
previously approved these changes
Mar 28, 2024
| @@ -197,4 +219,42 @@ public boolean redirectToLeader(BaseRequest request, BaseResponse response) thro | |||
| new TNetworkAddress(leaderIpAndPort.first, leaderIpAndPort.second)); | |||
| return true; | |||
| } | |||
|
|
|||
| protected static String getParameter(BaseRequest request, String paramName) { | |||
There was a problem hiding this comment.
while these getParameters are good help functions, thinking about a few improvements,
getParameter()return null if the paremter doesn't exist, it is up to the caller to determine what to do.getParameterRequired()throws BAD_REQUEST http exception if the parameter is not there.getParameterOrDefault()returns default value if the parameter is not there.- generic programming, getParameter for int/boolean/string/double/...,
org.apache.hadoop.conf.Configuration might be a good reference for these interfaces.
There was a problem hiding this comment.
@kevincai Good suggestions.
fe/fe-core/src/main/java/com/starrocks/http/rest/v2/vo/TablePartitionView.java
Outdated
Show resolved
Hide resolved
plotor
force-pushed
the
feature-table-schema-api-v2-zhenchao-20240227
branch
from
e91bd5b to
26e5330
Compare
gengjun-git
previously approved these changes
Mar 28, 2024
kevincai
previously approved these changes
Mar 28, 2024
satanson
previously approved these changes
Mar 28, 2024
jaogoy
reviewed
Mar 28, 2024
| .orElse(DEFAULT_INTERNAL_CATALOG_NAME).equalsIgnoreCase(catalogName)) { | ||
| throw new StarRocksHttpException( | ||
| HttpResponseStatus.NOT_FOUND, | ||
| String.format("Database[%s] does not exist in catalog %s", dbName, catalogName) |
There was a problem hiding this comment.
Will this message be shown to users?
If yes, put it into ErrorCode.java
There was a problem hiding this comment.
Yeah, and I found that I can reuse some existing ErrorCode.
| if (null == table) { | ||
| throw new StarRocksHttpException( | ||
| HttpResponseStatus.NOT_FOUND, | ||
| String.format("Table[%s.%s] does not exist in catalog %s", dbName, tableName, catalogName) |
|
|
||
| OlapTable olapTable = getOlapTable(catalogName, dbName, tableName); | ||
| Collection<Partition> partitions = | ||
| temporary ? olapTable.getTempPartitions() : olapTable.getPartitions(); |
There was a problem hiding this comment.
Do we need to lock the table to get a partition snapshot?
Or will the olapTable be modified when geting partitions.
There was a problem hiding this comment.
Yeah, It is a problem, and i encapsulated the processing logic of OlapTable into the db read lock.
wyb
reviewed
Mar 28, 2024
Comment on lines
74
to
88
| Collection<Partition> partitions = | ||
| temporary ? olapTable.getTempPartitions() : olapTable.getPartitions(); | ||
| if (CollectionUtils.isNotEmpty(partitions)) { | ||
| int pages = partitions.size() / pageSize; | ||
| pagedResult.setPages(partitions.size() % pageSize == 0 ? pages : (pages + 1)); | ||
| pagedResult.setTotal(partitions.size()); | ||
| pagedResult.setItems(partitions.stream() | ||
| .sorted(Comparator.comparingLong(Partition::getId)) | ||
| .skip((long) pageNum * pageSize) | ||
| .limit(pageSize) | ||
| .map(partition -> | ||
| TablePartitionView.createFrom(olapTable.getPartitionInfo(), partition)) | ||
| .collect(Collectors.toList()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Yeah, It is a problem, and i encapsulated the processing logic of OlapTable into the db read lock.
| String tableName = getSingleParameterRequired(request, TABLE_KEY, Function.identity()); | ||
|
|
||
| OlapTable olapTable = getOlapTable(catalogName, dbName, tableName); | ||
| sendResult(request, response, new RestBaseResultV2<>(TableSchemaView.createFrom(olapTable))); |
There was a problem hiding this comment.
Yeah, It is a problem, and i encapsulated the processing logic of OlapTable into the db read lock.
plotor
dismissed stale reviews from satanson, kevincai, and gengjun-git
via
f5acdcd
plotor
force-pushed
the
feature-table-schema-api-v2-zhenchao-20240227
branch
2 times, most recently
from
f5acdcd to
6271a96
Compare
wyb
previously approved these changes
Mar 29, 2024
jaogoy
reviewed
Mar 29, 2024
| @@ -110,7 +122,7 @@ public String getErrorRespWhenUnauthorized(AccessDeniedException accessDeniedExc | |||
| List<String> activatedRoles = authorizationMgr.getRoleNamesByRoleIds(context.getCurrentRoleIds()); | |||
| List<String> inactivatedRoles = | |||
| authorizationMgr.getInactivatedRoleNamesByUser(userIdentity, activatedRoles); | |||
| return "Access denied for user " + userIdentity + ". " + | |||
| return "Access denied for user " + userIdentity + ". " + | |||
There was a problem hiding this comment.
You can use ErrorCode.ERR_AUTHENTICATION_FAIL
There was a problem hiding this comment.
This change is caused by code formatting, and have nothing to do with the business logic of this PR. I have rolled it back to make this PR more focused.
| @@ -110,7 +122,7 @@ public String getErrorRespWhenUnauthorized(AccessDeniedException accessDeniedExc | |||
| List<String> activatedRoles = authorizationMgr.getRoleNamesByRoleIds(context.getCurrentRoleIds()); | |||
| List<String> inactivatedRoles = | |||
| authorizationMgr.getInactivatedRoleNamesByUser(userIdentity, activatedRoles); | |||
| return "Access denied for user " + userIdentity + ". " + | |||
| return "Access denied for user " + userIdentity + ". " + | |||
| String.format(ErrorCode.ERR_ACCESS_DENIED_HINT_MSG_FORMAT, activatedRoles, inactivatedRoles); | |||
| } | |||
| return "Access denied."; | |||
There was a problem hiding this comment.
Will it be too simple to let user know the error reason?
There was a problem hiding this comment.
this is intended to do so, because we cannot get any more specific info.
| } else if (dateLiteral.getType().isDatetime()) { | ||
| return dateLiteral.getLongValue(); | ||
| } else { | ||
| throw new StarRocksPlannerException("Invalid date type: " + dateLiteral.getType(), ErrorType.INTERNAL_ERROR); |
| import java.util.Optional; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class TablePartitionView { |
There was a problem hiding this comment.
can this class be merged with PartitionInfoView?
plotor
force-pushed
the
feature-table-schema-api-v2-zhenchao-20240227
branch
from
6271a96 to
9085e31
Compare
nshangyiming
approved these changes
Mar 29, 2024
gengjun-git
approved these changes
Mar 29, 2024
|
@mergify rebase |
… partition API. Signed-off-by: plotor <[email protected]>
✅ Branch has been successfully rebased |
nshangyiming
force-pushed
the
feature-table-schema-api-v2-zhenchao-20240227
branch
from
9085e31 to
58f48d0
Compare
|
[FE Incremental Coverage Report]✅ pass : 526 / 559 (94.10%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
meegoo
approved these changes
Apr 1, 2024
wyb
approved these changes
Apr 1, 2024
kevincai
approved these changes
Apr 1, 2024
Merged
24 tasks
8 tasks
24 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why I'm doing:
You can learn about the background from PR #38466.
What I'm doing:
This is a relatively big feature, and we will split it into multiple PRs. Related PRs include:
This PR is one of them, which mainly add a second version implementation of get table schema and partition API. This version of the API is designed to be as neutral as possible, and not tied to specific scenarios. Howeaver, there are still some usage restrictions to note:
OlapTable.The API of Get Table Schema and Partition are defined as follows:
Get Table Schema API Definition
/api/v2/catalogs/{catalog}/databases/{db}/tables/{table}/schemaGETdefault_catalogResponse Example:
{ "code": "0", "message": "OK", "result": { "id": 10200, "name": "tb_table_schema_test", "tableType": "OLAP", "keysType": "DUP_KEYS", "comment": "c_tb_table_schema_test", "createTime": 1709197545, "columns": [ { "name": "c1", "primitiveType": "DOUBLE", "primitiveTypeSize": 8, "columnSize": 15, "precision": 0, "scale": 0, "isKey": true, "isAllowNull": false, "isAutoIncrement": false, "defaultValueType": "NULL", "comment": "cc1", "uniqueId": 1 }, { "name": "c2", "primitiveType": "DECIMAL64", "primitiveTypeSize": 8, "columnSize": 18, "precision": 18, "scale": 6, "aggregationType": "SUM", "isKey": false, "isAllowNull": true, "isAutoIncrement": false, "defaultValueType": "CONST", "defaultValue": "0", "comment": "cc2", "uniqueId": 2 } ], "indexMetas": [ { "indexId": 10200, "keysType": "DUP_KEYS", "columns": [ { "name": "c1", "primitiveType": "DOUBLE", "primitiveTypeSize": 8, "columnSize": 15, "precision": 0, "scale": 0, "isKey": true, "isAllowNull": false, "isAutoIncrement": false, "defaultValueType": "NULL", "comment": "cc1", "uniqueId": 1 } ] } ], "partitionInfo": { "type": "RANGE", "partitionColumns": [ { "name": "c1", "primitiveType": "DOUBLE", "primitiveTypeSize": 8, "columnSize": 15, "precision": 0, "scale": 0, "isKey": true, "isAllowNull": false, "isAutoIncrement": false, "defaultValueType": "NULL", "comment": "cc1", "uniqueId": 1 } ] }, "defaultDistributionInfo": { "type": "HASH", "bucketNum": 8, "distributionColumns": [ { "name": "c1", "primitiveType": "DOUBLE", "primitiveTypeSize": 8, "columnSize": 15, "precision": 0, "scale": 0, "isKey": true, "isAllowNull": false, "isAutoIncrement": false, "defaultValueType": "NULL", "comment": "cc1", "uniqueId": 1 } ] }, "colocateGroup": "cg1", "indexes": [ { "indexId": 10200, "indexName": "idx1", "indexType": "BITMAP", "columns": [ "c1" ], "comment": "c_idx1", "properties": {} }, { "indexId": 10201, "indexName": "idx2", "indexType": "NGRAMBF", "columns": [ "c2" ], "comment": "c_idx2", "properties": {} } ], "baseIndexId": 10200, "maxIndexId": 75735, "maxColUniqueId": 65535, "properties": { "replication_num": "3", "colocate_with": "cg1" } } }Get Table Partition API Definition
/api/v2/catalogs/{catalog}/databases/{db}/tables/{table}/partitionGETdefault_catalogtrue, If set to true, it means get temporary partition infoThe response template is as follows:
{ "code": "0", "message": "OK", "result": { "pageNum": 1, "pageSize": 7, "pages": 1, "total": 7, "items": [] } }Parameter description:
pageNum: the page number in request.pageSize: the page size in request.pages: total page size.total: total item size.items: data items.Response Example:
{ "code": "0", "message": "OK", "result": { "pageNum": 1, "pageSize": 3, "pages": 3, "total": 7, "items": [ { "id": 11204, "name": "testPartition_3", "bucketNum": 8, "distributionType": "HASH", "visibleVersion": 12, "visibleVersionTime": 1711351506629, "nextVersion": 13, "isMinPartition": false, "isMaxPartition": false, "startKeys": [ 30.0 ], "endKeys": [ 40.0 ], "tablets": [ { "id": 400, "backendIds": [ 1000, 1001, 1002 ] } ] }, { "id": 11205, "name": "testPartition_4", "bucketNum": 8, "distributionType": "HASH", "visibleVersion": 12, "visibleVersionTime": 1711351506629, "nextVersion": 13, "isMinPartition": false, "isMaxPartition": false, "startKeys": [ 40.0 ], "endKeys": [ 50.0 ], "tablets": [ { "id": 400, "backendIds": [ 1000, 1001, 1002 ] } ] }, { "id": 11206, "name": "testPartition_5", "bucketNum": 8, "distributionType": "HASH", "visibleVersion": 12, "visibleVersionTime": 1711351506629, "nextVersion": 13, "isMinPartition": false, "isMaxPartition": false, "startKeys": [ 50.0 ], "endKeys": [ 60.0 ], "tablets": [ { "id": 400, "backendIds": [ 1000, 1001, 1002 ] } ] } ] } }Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: