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/02/18 17:23:08 UTC
[GitHub] [iceberg] rajarshisarkar opened a new pull request #4166: AWS: Use Glue optimistic locking while updating table
rajarshisarkar opened a new pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166
Draft PR to use Glue optimistic locking while updating table.
--
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 #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
jackye1995 merged pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166
--
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] rajarshisarkar commented on a change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r810885773
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
##########
@@ -63,6 +64,12 @@
private final FileIO fileIO;
private final LockManager lockManager;
+ // Attempt to load versionId if available on the path
Review comment:
I have made the changes.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
##########
@@ -194,8 +201,12 @@ void persistGlueTable(Table glueTable, Map<String, String> parameters, TableMeta
.name(tableName)
.tableType(GLUE_EXTERNAL_TABLE_TYPE)
.parameters(parameters)
- .build())
- .build());
+ .build());
+ // Use Optimistic locking with version id while updating table
+ if(!LOAD_VERSION_ID.isNoop()) {
+ updateTableRequest.versionId(glueTable.versionId());
+ }
+ glue.updateTable(updateTableRequest.build());
Review comment:
I have made the changes.
--
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 change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r810653150
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
##########
@@ -63,6 +64,12 @@
private final FileIO fileIO;
private final LockManager lockManager;
+ // Attempt to load versionId if available on the path
+ private static final DynMethods.UnboundMethod LOAD_VERSION_ID = DynMethods.builder("versionId")
+ .hiddenImpl("software.amazon.awssdk.services.glue.model.UpdateTableRequest$Builder", String.class)
+ .orNoop()
+ .build();
Review comment:
It's a little unclear what this method is doing. It looks like this sets a requirement that the table has not changed since the given version ID? If so, then I think the comment below could be more clear.
--
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 pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#issuecomment-1046276436
> Now we don't really need a lock manager if lock-impl is not set.
True, but since this is using reflection and may not actually call `versionId` I think we need to be careful. We could check whether `LOAD_VERSION_ID` is a noop and require a lock implementation.
--
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 change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r810653025
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
##########
@@ -194,8 +201,12 @@ void persistGlueTable(Table glueTable, Map<String, String> parameters, TableMeta
.name(tableName)
.tableType(GLUE_EXTERNAL_TABLE_TYPE)
.parameters(parameters)
- .build())
- .build());
+ .build());
+ // Use Optimistic locking with version id while updating table
+ if (!LOAD_VERSION_ID.isNoop()) {
+ updateTableRequest.versionId(glueTable.versionId());
Review comment:
Why not use reflection to call the method as well? That seems slightly safer, although I guess it is fine as long as you've tested this with older SDK 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.
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] rajarshisarkar commented on a change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r810890049
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
##########
@@ -63,6 +64,12 @@
private final FileIO fileIO;
private final LockManager lockManager;
+ // Attempt to load versionId if available on the path
+ private static final DynMethods.UnboundMethod LOAD_VERSION_ID = DynMethods.builder("versionId")
+ .hiddenImpl("software.amazon.awssdk.services.glue.model.UpdateTableRequest$Builder", String.class)
+ .orNoop()
+ .build();
Review comment:
Yes, correct.
```
/**
* Sets the value of the VersionId property for this object.
*
* @param versionId
* The new value for the VersionId property for this object.
* @return Returns a reference to this object so that method calls can be chained together.
*/
Builder versionId(String versionId);
```
--
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 change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r811369798
##########
File path: core/src/main/java/org/apache/iceberg/util/LockManagers.java
##########
@@ -54,6 +55,15 @@ public static LockManager from(Map<String, String> properties) {
}
}
+ public static LockManager from(DynMethods.UnboundMethod setVersionId, Map<String, String> properties) {
Review comment:
I think we don't need to do this in the core library, we can just apply the logic in the GlueCatalog when initializing the catalog. This is too specific implementation detail for the GlueCatalog.
--
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 change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r810251892
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
##########
@@ -194,8 +201,12 @@ void persistGlueTable(Table glueTable, Map<String, String> parameters, TableMeta
.name(tableName)
.tableType(GLUE_EXTERNAL_TABLE_TYPE)
.parameters(parameters)
- .build())
- .build());
+ .build());
+ // Use Optimistic locking with version id while updating table
+ if(!LOAD_VERSION_ID.isNoop()) {
+ updateTableRequest.versionId(glueTable.versionId());
+ }
+ glue.updateTable(updateTableRequest.build());
Review comment:
nit: newline after if block
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
##########
@@ -63,6 +64,12 @@
private final FileIO fileIO;
private final LockManager lockManager;
+ // Attempt to load versionId if available on the path
Review comment:
I think it's not "load", but "set" the version ID?
--
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 change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r812147065
##########
File path: core/src/main/java/org/apache/iceberg/util/LockManagers.java
##########
@@ -37,7 +37,7 @@
public class LockManagers {
- private static final LockManager LOCK_MANAGER_DEFAULT = new InMemoryLockManager(Maps.newHashMap());
Review comment:
there is already a `defaultLockManager` method below exposing this, we don't need to make it public
--
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 change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r811429092
##########
File path: core/src/main/java/org/apache/iceberg/util/LockManagers.java
##########
@@ -54,6 +55,15 @@ public static LockManager from(Map<String, String> properties) {
}
}
+ public static LockManager from(DynMethods.UnboundMethod setVersionId, Map<String, String> properties) {
Review comment:
Agreed!
--
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] rajarshisarkar commented on a change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r810886933
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
##########
@@ -194,8 +201,12 @@ void persistGlueTable(Table glueTable, Map<String, String> parameters, TableMeta
.name(tableName)
.tableType(GLUE_EXTERNAL_TABLE_TYPE)
.parameters(parameters)
- .build())
- .build());
+ .build());
+ // Use Optimistic locking with version id while updating table
+ if (!LOAD_VERSION_ID.isNoop()) {
+ updateTableRequest.versionId(glueTable.versionId());
Review comment:
I agree, used reflection to invoke the method.
--
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 change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r810652668
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
##########
@@ -194,8 +201,12 @@ void persistGlueTable(Table glueTable, Map<String, String> parameters, TableMeta
.name(tableName)
.tableType(GLUE_EXTERNAL_TABLE_TYPE)
.parameters(parameters)
- .build())
- .build());
+ .build());
+ // Use Optimistic locking with version id while updating table
+ if(!LOAD_VERSION_ID.isNoop()) {
+ updateTableRequest.versionId(glueTable.versionId());
+ }
+ glue.updateTable(updateTableRequest.build());
Review comment:
And a space between `if` and `(`.
--
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 #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#issuecomment-1044980909
cc @arminnajafi @singhpk234 @amogh-jahagirdar @xiaoxuandev @yyanyy @natsukawa-kanou
--
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] rajarshisarkar commented on pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#issuecomment-1046645510
> Now we don't really need a lock manager if lock-impl is not set. I think using a in-memory lock is also not needed.
If we implement a no-op lock manager for GlueCatalog and use that instead of the default in-memory lock manager if user did not specify any lock configuration. Any thoughts about this?
Yes, we shouldn't require any lock managers unless we encounter the noop scenario. We can fallback to in-memory lock manager in that case.
> True, but since this is using reflection and may not actually call versionId I think we need to be careful. We could check whether LOAD_VERSION_ID is a noop and require a lock implementation.
Yes, I agree.
--
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] rajarshisarkar commented on a change in pull request #4166: AWS: Use Glue optimistic locking while updating table
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4166:
URL: https://github.com/apache/iceberg/pull/4166#discussion_r811815529
##########
File path: core/src/main/java/org/apache/iceberg/util/LockManagers.java
##########
@@ -54,6 +55,15 @@ public static LockManager from(Map<String, String> properties) {
}
}
+ public static LockManager from(DynMethods.UnboundMethod setVersionId, Map<String, String> properties) {
Review comment:
Thanks, I have made the changes.
--
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