You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/12/05 06:17:50 UTC
[GitHub] [iceberg] aajisaka opened a new pull request, #6358: AWS: Print debug log when Glue optimistic locking is used
aajisaka opened a new pull request, #6358:
URL: https://github.com/apache/iceberg/pull/6358
### Background
Glue optimistic locking is automatically enabled if aws-java-sdk version is >= 2.17.131, however, users sometimes cannot check what version of aws-java-sdk is actually used in the runtime environment. Therefore I suppose it's good to add logging for users to understand whether the optimistic locking is actually enabled or not.
--
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] aajisaka commented on pull request #6358: AWS: Print logs whether Glue optimistic locking is used or not
Posted by GitBox <gi...@apache.org>.
aajisaka commented on PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#issuecomment-1396566666
Can someone review the latest version?
--
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] rdblue commented on a diff in pull request #6358: AWS: Print logs whether Glue optimistic locking is used or not
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#discussion_r1051686720
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -315,7 +315,12 @@ void persistGlueTable(Table glueTable, Map<String, String> parameters, TableMeta
.build());
// Use Optimistic locking with table version id while updating table
if (!SET_VERSION_ID.isNoop() && lockManager == null) {
+ LOG.debug("Using optimistic locking with table version: {}", glueTable.versionId());
Review Comment:
Configuration should be logged once, not every time it is used. Can you fix this to log what you want to when the catalog is 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
[GitHub] [iceberg] jackye1995 merged pull request #6358: AWS: Print logs whether Glue optimistic locking is used or not
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #6358:
URL: https://github.com/apache/iceberg/pull/6358
--
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 a diff in pull request #6358: AWS: Print logs whether Glue optimistic locking is used or not
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#discussion_r1081579915
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -151,7 +151,12 @@ private LockManager initializeLockManager(Map<String, String> properties) {
if (properties.containsKey(CatalogProperties.LOCK_IMPL)) {
return LockManagers.from(properties);
} else if (SET_VERSION_ID.isNoop()) {
+ LOG.warn(
+ "Optimistic locking is not available in the environment. Using in-memory lock manager."
+ + " To ensure atomic transaction, you need to setup a DynamoDB lock manager.");
Review Comment:
Nit: generally in log messages I think we try to avoid the second person.
"To ensure atomic transaction, please configure a distributed lock manager such as the DynamoDB lock manager"
--
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] aajisaka commented on pull request #6358: AWS: Print debug log when Glue optimistic locking is used
Posted by GitBox <gi...@apache.org>.
aajisaka commented on PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#issuecomment-1340517611
Hi @jackye1995 would you 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] amogh-jahagirdar commented on a diff in pull request #6358: AWS: Print logs whether Glue optimistic locking is used or not
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#discussion_r1081579915
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -151,7 +151,12 @@ private LockManager initializeLockManager(Map<String, String> properties) {
if (properties.containsKey(CatalogProperties.LOCK_IMPL)) {
return LockManagers.from(properties);
} else if (SET_VERSION_ID.isNoop()) {
+ LOG.warn(
+ "Optimistic locking is not available in the environment. Using in-memory lock manager."
+ + " To ensure atomic transaction, you need to setup a DynamoDB lock manager.");
Review Comment:
Nit: generally in log messages I think we try to avoid the second person.
"To ensure atomic transactions, please configure a distributed lock manager such as the DynamoDB lock manager"
--
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] aajisaka commented on a diff in pull request #6358: AWS: Print debug log when Glue optimistic locking is used
Posted by GitBox <gi...@apache.org>.
aajisaka commented on code in PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#discussion_r1046683973
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -315,6 +315,7 @@ void persistGlueTable(Table glueTable, Map<String, String> parameters, TableMeta
.build());
// Use Optimistic locking with table version id while updating table
if (!SET_VERSION_ID.isNoop() && lockManager == null) {
+ LOG.debug("Use optimistic locking with table version: {}", glueTable.versionId());
Review Comment:
Thanks. Fixed.
--
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] aajisaka commented on a diff in pull request #6358: AWS: Print logs whether Glue optimistic locking is used or not
Posted by GitBox <gi...@apache.org>.
aajisaka commented on code in PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#discussion_r1082137636
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -151,7 +151,12 @@ private LockManager initializeLockManager(Map<String, String> properties) {
if (properties.containsKey(CatalogProperties.LOCK_IMPL)) {
return LockManagers.from(properties);
} else if (SET_VERSION_ID.isNoop()) {
+ LOG.warn(
+ "Optimistic locking is not available in the environment. Using in-memory lock manager."
+ + " To ensure atomic transaction, you need to setup a DynamoDB lock manager.");
Review Comment:
Fixed the warn message as you suggested. Thank you @amogh-jahagirdar
--
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] aajisaka commented on pull request #6358: AWS: Print logs whether Glue optimistic locking is used or not
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#issuecomment-1402389457
Thank you for the reviews @rdblue @jackye1995 @amogh-jahagirdar
--
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 a diff in pull request #6358: AWS: Print debug log when Glue optimistic locking is used
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#discussion_r1043717687
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -315,6 +315,7 @@ void persistGlueTable(Table glueTable, Map<String, String> parameters, TableMeta
.build());
// Use Optimistic locking with table version id while updating table
if (!SET_VERSION_ID.isNoop() && lockManager == null) {
+ LOG.debug("Use optimistic locking with table version: {}", glueTable.versionId());
Review Comment:
Nit: Use -> Using
--
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] aajisaka commented on a diff in pull request #6358: AWS: Print logs whether Glue optimistic locking is used or not
Posted by GitBox <gi...@apache.org>.
aajisaka commented on code in PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#discussion_r1052334835
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -315,7 +315,12 @@ void persistGlueTable(Table glueTable, Map<String, String> parameters, TableMeta
.build());
// Use Optimistic locking with table version id while updating table
if (!SET_VERSION_ID.isNoop() && lockManager == null) {
+ LOG.debug("Using optimistic locking with table version: {}", glueTable.versionId());
Review Comment:
Thank you for your comment! Reflected in the latest commit.
--
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] aajisaka commented on pull request #6358: AWS: Print debug log when Glue optimistic locking is used
Posted by GitBox <gi...@apache.org>.
aajisaka commented on PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#issuecomment-1347782434
> IMO I also think we should have a warn log if no lock manager is passed in and optimistic locking is not used.
Nice catch! Added warn log.
--
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 #6358: AWS: Print logs whether Glue optimistic locking is used or not
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #6358:
URL: https://github.com/apache/iceberg/pull/6358#issuecomment-1402156201
Thanks for the fix @aajisaka , thanks for the reviews @rdblue and @amogh-jahagirdar !
--
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