You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "munendrasn (via GitHub)" <gi...@apache.org> on 2023/02/13 14:55:53 UTC
[GitHub] [iceberg] munendrasn opened a new pull request, #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
munendrasn opened a new pull request, #6823:
URL: https://github.com/apache/iceberg/pull/6823
While working on issue #6763 found that lastEvaluatedKey was not set for subsequent calls in listTables()
@jackye1995 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6823:
URL: https://github.com/apache/iceberg/pull/6823#discussion_r1105277157
##########
aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java:
##########
@@ -329,7 +329,7 @@ public boolean removeProperties(Namespace namespace, Set<String> properties)
@Override
public List<TableIdentifier> listTables(Namespace namespace) {
List<TableIdentifier> identifiers = Lists.newArrayList();
- Map<String, AttributeValue> lastEvaluatedKey;
+ Map<String, AttributeValue> lastEvaluatedKey = null;
Review Comment:
I see, then it looks good to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #6823:
URL: https://github.com/apache/iceberg/pull/6823#issuecomment-1433573967
Thanks for the fix! @munendrasn
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] munendrasn commented on pull request #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on PR #6823:
URL: https://github.com/apache/iceberg/pull/6823#issuecomment-1429660556
I thought of updating this [test](https://github.com/apache/iceberg/blob/ef7e20e12eae2638015ff91b3356eb2f32f601cc/aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java#L213) to create more tables, and validate the change.
As DynamoDb Query reads max 1MB of data (when limit is not specified), need to create large number of tables (~10K with current setup) to reproduce the issue. Please let me know if I should go ahead, and update number of tables to higher number in the tests
Locally, I could verify the fix by adding `limit` parameter to the query request and then calling listTables on DynamoDb with & w/o the fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on pull request #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #6823:
URL: https://github.com/apache/iceberg/pull/6823#issuecomment-1430239347
+1 for adding some more AWS integration tests specifically focused on scale 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #6823:
URL: https://github.com/apache/iceberg/pull/6823
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] munendrasn commented on pull request #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on PR #6823:
URL: https://github.com/apache/iceberg/pull/6823#issuecomment-1435041793
Thank you all for the review 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #6823:
URL: https://github.com/apache/iceberg/pull/6823#issuecomment-1429097423
Could you add an integration test about 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6823:
URL: https://github.com/apache/iceberg/pull/6823#discussion_r1104867442
##########
aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java:
##########
@@ -329,7 +329,7 @@ public boolean removeProperties(Namespace namespace, Set<String> properties)
@Override
public List<TableIdentifier> listTables(Namespace namespace) {
List<TableIdentifier> identifiers = Lists.newArrayList();
- Map<String, AttributeValue> lastEvaluatedKey;
+ Map<String, AttributeValue> lastEvaluatedKey = null;
Review Comment:
I don't think this change is needed? if not assigned it's default to null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] munendrasn commented on a diff in pull request #6823: AWS: set lastEvaluatedKey for listTables in DynamoDb Catalog
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on code in PR #6823:
URL: https://github.com/apache/iceberg/pull/6823#discussion_r1105253250
##########
aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java:
##########
@@ -329,7 +329,7 @@ public boolean removeProperties(Namespace namespace, Set<String> properties)
@Override
public List<TableIdentifier> listTables(Namespace namespace) {
List<TableIdentifier> identifiers = Lists.newArrayList();
- Map<String, AttributeValue> lastEvaluatedKey;
+ Map<String, AttributeValue> lastEvaluatedKey = null;
Review Comment:
For local variables, if used without initialisation, compilation fails (using Jdk8). `error: variable lastEvaluatedKey might not have been initialized`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org