yyanyy commented on a change in pull request #1823:
URL: https://github.com/apache/iceberg/pull/1823#discussion_r530684956



##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -114,6 +115,33 @@
    */
   public static final String S3FILEIO_ACL = "s3fileio.acl";
 
+  /**
+   * If {@link org.apache.iceberg.aws.glue.GlueCatalog} should use external 
lock or not, default to false.
+   * If set to true, it will use DynamoDB to enforce locking during commits.
+   */
+  public static final String GLUE_CATALOG_LOCK_ENABLED = 
"gluecatalog.lock.enabled";
+  public static final boolean GLUE_CATALOG_LOCK_ENABLED_DEFAULT = false;
+
+  /**
+   * The DynamoDB table used for locking.
+   * One lock table is designed to be used for only one catalog.
+   * It is recommended to use a different table name for each Glue catalog.

Review comment:
       Is "One lock table is designed to be used for only one catalog" still 
accurate? It seems to me that we are able to have one lock table controlling 
multiple catalogs? And I think if we do want to recommend one table per 
catalog, we probably want to update the default to be prefix + catalog name to 
follow our own recommendation just as Ryan commented above. Although I'm not 
super clear on what exactly is the benefit of having one ddb table per 
catalog...

##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/DynamoLockManager.java
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws.glue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.aws.AwsClientUtil;
+import org.apache.iceberg.aws.AwsProperties;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import software.amazon.awssdk.services.dynamodb.DynamoDbClient;
+import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition;
+import software.amazon.awssdk.services.dynamodb.model.AttributeValue;
+import software.amazon.awssdk.services.dynamodb.model.BillingMode;
+import software.amazon.awssdk.services.dynamodb.model.CreateTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DeleteItemRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableResponse;
+import software.amazon.awssdk.services.dynamodb.model.KeySchemaElement;
+import software.amazon.awssdk.services.dynamodb.model.KeyType;
+import software.amazon.awssdk.services.dynamodb.model.PutItemRequest;
+import 
software.amazon.awssdk.services.dynamodb.model.ResourceNotFoundException;
+import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType;
+import software.amazon.awssdk.services.dynamodb.model.TableStatus;
+
+class DynamoLockManager implements LockManager {
+
+  private static final Logger LOG = 
LoggerFactory.getLogger(DynamoLockManager.class);
+
+  private static final String LOCK_TABLE_COL_TABLE_ID = "tableId";
+  private static final String LOCK_TABLE_COL_EXPIRE_TS_MILLIS = 
"expireTimestampMillis";
+
+  private static final List<KeySchemaElement> LOCK_TABLE_SCHEMA = 
Lists.newArrayList(
+      KeySchemaElement.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .keyType(KeyType.HASH)
+          .build()
+  );
+
+  private static final List<AttributeDefinition> LOCK_TABLE_COL_DEFINITIONS = 
Lists.newArrayList(
+      AttributeDefinition.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .attributeType(ScalarAttributeType.S)
+          .build()
+  );
+
+  private final DynamoDbClient dynamo;
+  private final AwsProperties awsProperties;
+
+  DynamoLockManager(AwsProperties awsProperties) {
+    this(AwsClientUtil.defaultDynamoClient(), awsProperties);
+  }
+
+  DynamoLockManager(DynamoDbClient dynamo, AwsProperties awsProperties) {
+    this.dynamo = dynamo;
+    this.awsProperties = awsProperties;
+    ensureLockTableExists();
+  }
+
+  private void ensureLockTableExists() {
+    ensureTableExists(awsProperties.glueCatalogLockTable(), LOCK_TABLE_SCHEMA, 
LOCK_TABLE_COL_DEFINITIONS);
+  }
+
+  private void ensureTableExists(String tableName, List<KeySchemaElement> 
schema,
+                                 List<AttributeDefinition> definitions) {
+    try {
+      dynamo.describeTable(DescribeTableRequest.builder()
+          .tableName(tableName)
+          .build());
+    } catch (ResourceNotFoundException e) {
+      LOG.info("Glue lock DynamoDB table <{}> not found, trying to create", 
tableName);
+      dynamo.createTable(CreateTableRequest.builder()
+          .tableName(tableName)
+          .keySchema(schema)
+          .attributeDefinitions(definitions)
+          .billingMode(BillingMode.PAY_PER_REQUEST)
+          .build());
+
+      boolean isTableActive = false;
+      while (!isTableActive) {
+        LOG.info("waiting for DynamoDB table <{}> to be active", tableName);
+        try {
+          Thread.sleep(5000);
+        } catch (InterruptedException ie) {
+          LOG.warn("Glue lock DynamoDB table creation sleep interrupted", e);
+        }
+        DescribeTableResponse describeTableResponse = 
dynamo.describeTable(DescribeTableRequest.builder()
+            .tableName(tableName)
+            .build());
+        isTableActive = 
describeTableResponse.table().tableStatus().equals(TableStatus.ACTIVE);
+      }
+    }
+  }
+
+
+  @Override
+  public boolean tryLock(String database, String table, long expireMillis) {
+    String tableId = tableId(database, table);
+
+    Map<String, AttributeValue> key = Maps.newHashMap();
+    key.put(LOCK_TABLE_COL_TABLE_ID, 
AttributeValue.builder().s(tableId).build());
+    key.put(LOCK_TABLE_COL_EXPIRE_TS_MILLIS, AttributeValue.builder().n(
+        Long.toString(System.currentTimeMillis() + expireMillis)

Review comment:
       I think `System.currentTimeMillis` may lead to [clock skew 
problem](https://stackoverflow.com/questions/2978598/will-system-currenttimemillis-always-return-a-value-previous-calls/2979239#2979239).
 DDBLockClient's implementation to workaround this problem is interesting, 
probably worth borrowing: 
https://github.com/awslabs/amazon-dynamodb-lock-client#how-we-handle-clock-skew

##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/DynamoLockManager.java
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws.glue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.aws.AwsClientUtil;
+import org.apache.iceberg.aws.AwsProperties;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import software.amazon.awssdk.services.dynamodb.DynamoDbClient;
+import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition;
+import software.amazon.awssdk.services.dynamodb.model.AttributeValue;
+import software.amazon.awssdk.services.dynamodb.model.BillingMode;
+import software.amazon.awssdk.services.dynamodb.model.CreateTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DeleteItemRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableResponse;
+import software.amazon.awssdk.services.dynamodb.model.KeySchemaElement;
+import software.amazon.awssdk.services.dynamodb.model.KeyType;
+import software.amazon.awssdk.services.dynamodb.model.PutItemRequest;
+import 
software.amazon.awssdk.services.dynamodb.model.ResourceNotFoundException;
+import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType;
+import software.amazon.awssdk.services.dynamodb.model.TableStatus;
+
+class DynamoLockManager implements LockManager {
+
+  private static final Logger LOG = 
LoggerFactory.getLogger(DynamoLockManager.class);
+
+  private static final String LOCK_TABLE_COL_TABLE_ID = "tableId";
+  private static final String LOCK_TABLE_COL_EXPIRE_TS_MILLIS = 
"expireTimestampMillis";
+
+  private static final List<KeySchemaElement> LOCK_TABLE_SCHEMA = 
Lists.newArrayList(
+      KeySchemaElement.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .keyType(KeyType.HASH)
+          .build()
+  );
+
+  private static final List<AttributeDefinition> LOCK_TABLE_COL_DEFINITIONS = 
Lists.newArrayList(
+      AttributeDefinition.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .attributeType(ScalarAttributeType.S)
+          .build()
+  );
+
+  private final DynamoDbClient dynamo;
+  private final AwsProperties awsProperties;
+
+  DynamoLockManager(AwsProperties awsProperties) {
+    this(AwsClientUtil.defaultDynamoClient(), awsProperties);
+  }
+
+  DynamoLockManager(DynamoDbClient dynamo, AwsProperties awsProperties) {
+    this.dynamo = dynamo;
+    this.awsProperties = awsProperties;
+    ensureLockTableExists();
+  }
+
+  private void ensureLockTableExists() {
+    ensureTableExists(awsProperties.glueCatalogLockTable(), LOCK_TABLE_SCHEMA, 
LOCK_TABLE_COL_DEFINITIONS);
+  }
+
+  private void ensureTableExists(String tableName, List<KeySchemaElement> 
schema,
+                                 List<AttributeDefinition> definitions) {
+    try {
+      dynamo.describeTable(DescribeTableRequest.builder()
+          .tableName(tableName)
+          .build());
+    } catch (ResourceNotFoundException e) {
+      LOG.info("Glue lock DynamoDB table <{}> not found, trying to create", 
tableName);
+      dynamo.createTable(CreateTableRequest.builder()
+          .tableName(tableName)
+          .keySchema(schema)
+          .attributeDefinitions(definitions)
+          .billingMode(BillingMode.PAY_PER_REQUEST)
+          .build());
+
+      boolean isTableActive = false;
+      while (!isTableActive) {
+        LOG.info("waiting for DynamoDB table <{}> to be active", tableName);
+        try {
+          Thread.sleep(5000);
+        } catch (InterruptedException ie) {
+          LOG.warn("Glue lock DynamoDB table creation sleep interrupted", e);
+        }
+        DescribeTableResponse describeTableResponse = 
dynamo.describeTable(DescribeTableRequest.builder()

Review comment:
       I think we may still need to catch `ResourceNotFoundException`  here: 
https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_DescribeTable.html

##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/DynamoLockManager.java
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws.glue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.aws.AwsClientUtil;
+import org.apache.iceberg.aws.AwsProperties;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import software.amazon.awssdk.services.dynamodb.DynamoDbClient;
+import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition;
+import software.amazon.awssdk.services.dynamodb.model.AttributeValue;
+import software.amazon.awssdk.services.dynamodb.model.BillingMode;
+import software.amazon.awssdk.services.dynamodb.model.CreateTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DeleteItemRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableResponse;
+import software.amazon.awssdk.services.dynamodb.model.KeySchemaElement;
+import software.amazon.awssdk.services.dynamodb.model.KeyType;
+import software.amazon.awssdk.services.dynamodb.model.PutItemRequest;
+import 
software.amazon.awssdk.services.dynamodb.model.ResourceNotFoundException;
+import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType;
+import software.amazon.awssdk.services.dynamodb.model.TableStatus;
+
+class DynamoLockManager implements LockManager {
+
+  private static final Logger LOG = 
LoggerFactory.getLogger(DynamoLockManager.class);
+
+  private static final String LOCK_TABLE_COL_TABLE_ID = "tableId";
+  private static final String LOCK_TABLE_COL_EXPIRE_TS_MILLIS = 
"expireTimestampMillis";
+
+  private static final List<KeySchemaElement> LOCK_TABLE_SCHEMA = 
Lists.newArrayList(
+      KeySchemaElement.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .keyType(KeyType.HASH)
+          .build()
+  );
+
+  private static final List<AttributeDefinition> LOCK_TABLE_COL_DEFINITIONS = 
Lists.newArrayList(
+      AttributeDefinition.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .attributeType(ScalarAttributeType.S)
+          .build()
+  );
+
+  private final DynamoDbClient dynamo;
+  private final AwsProperties awsProperties;
+
+  DynamoLockManager(AwsProperties awsProperties) {
+    this(AwsClientUtil.defaultDynamoClient(), awsProperties);
+  }
+
+  DynamoLockManager(DynamoDbClient dynamo, AwsProperties awsProperties) {
+    this.dynamo = dynamo;
+    this.awsProperties = awsProperties;
+    ensureLockTableExists();
+  }
+
+  private void ensureLockTableExists() {
+    ensureTableExists(awsProperties.glueCatalogLockTable(), LOCK_TABLE_SCHEMA, 
LOCK_TABLE_COL_DEFINITIONS);
+  }
+
+  private void ensureTableExists(String tableName, List<KeySchemaElement> 
schema,
+                                 List<AttributeDefinition> definitions) {
+    try {
+      dynamo.describeTable(DescribeTableRequest.builder()
+          .tableName(tableName)
+          .build());
+    } catch (ResourceNotFoundException e) {
+      LOG.info("Glue lock DynamoDB table <{}> not found, trying to create", 
tableName);
+      dynamo.createTable(CreateTableRequest.builder()
+          .tableName(tableName)
+          .keySchema(schema)
+          .attributeDefinitions(definitions)
+          .billingMode(BillingMode.PAY_PER_REQUEST)
+          .build());
+
+      boolean isTableActive = false;
+      while (!isTableActive) {

Review comment:
       I think we might want some timeout (hard coded probably could be fine) 
to avoid staying in the loop forever

##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -114,6 +115,33 @@
    */
   public static final String S3FILEIO_ACL = "s3fileio.acl";
 
+  /**
+   * If {@link org.apache.iceberg.aws.glue.GlueCatalog} should use external 
lock or not, default to false.
+   * If set to true, it will use DynamoDB to enforce locking during commits.
+   */
+  public static final String GLUE_CATALOG_LOCK_ENABLED = 
"gluecatalog.lock.enabled";
+  public static final boolean GLUE_CATALOG_LOCK_ENABLED_DEFAULT = false;

Review comment:
       +1, also I'm not an expert in AWS free tier but according to 
[this](https://aws.amazon.com/free/?all-free-tier.sort-by=item.additionalFields.SortRank&all-free-tier.sort-order=asc&all-free-tier.q=dynamodb&all-free-tier.q_operator=AND)
 it seems like low provisioned capacity counts towards free tier, so charging 
might not be a big problem in normal use cases. If this is the case, I guess 
this also applies to how table gets set up (`billingMode`).

##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/DynamoLockManager.java
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws.glue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.aws.AwsClientUtil;
+import org.apache.iceberg.aws.AwsProperties;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import software.amazon.awssdk.services.dynamodb.DynamoDbClient;
+import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition;
+import software.amazon.awssdk.services.dynamodb.model.AttributeValue;
+import software.amazon.awssdk.services.dynamodb.model.BillingMode;
+import software.amazon.awssdk.services.dynamodb.model.CreateTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DeleteItemRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableResponse;
+import software.amazon.awssdk.services.dynamodb.model.KeySchemaElement;
+import software.amazon.awssdk.services.dynamodb.model.KeyType;
+import software.amazon.awssdk.services.dynamodb.model.PutItemRequest;
+import 
software.amazon.awssdk.services.dynamodb.model.ResourceNotFoundException;
+import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType;
+import software.amazon.awssdk.services.dynamodb.model.TableStatus;
+
+class DynamoLockManager implements LockManager {
+
+  private static final Logger LOG = 
LoggerFactory.getLogger(DynamoLockManager.class);
+
+  private static final String LOCK_TABLE_COL_TABLE_ID = "tableId";
+  private static final String LOCK_TABLE_COL_EXPIRE_TS_MILLIS = 
"expireTimestampMillis";
+
+  private static final List<KeySchemaElement> LOCK_TABLE_SCHEMA = 
Lists.newArrayList(
+      KeySchemaElement.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .keyType(KeyType.HASH)
+          .build()
+  );
+
+  private static final List<AttributeDefinition> LOCK_TABLE_COL_DEFINITIONS = 
Lists.newArrayList(
+      AttributeDefinition.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .attributeType(ScalarAttributeType.S)
+          .build()
+  );
+
+  private final DynamoDbClient dynamo;
+  private final AwsProperties awsProperties;
+
+  DynamoLockManager(AwsProperties awsProperties) {
+    this(AwsClientUtil.defaultDynamoClient(), awsProperties);
+  }
+
+  DynamoLockManager(DynamoDbClient dynamo, AwsProperties awsProperties) {
+    this.dynamo = dynamo;
+    this.awsProperties = awsProperties;
+    ensureLockTableExists();
+  }
+
+  private void ensureLockTableExists() {
+    ensureTableExists(awsProperties.glueCatalogLockTable(), LOCK_TABLE_SCHEMA, 
LOCK_TABLE_COL_DEFINITIONS);
+  }
+
+  private void ensureTableExists(String tableName, List<KeySchemaElement> 
schema,
+                                 List<AttributeDefinition> definitions) {
+    try {
+      dynamo.describeTable(DescribeTableRequest.builder()
+          .tableName(tableName)
+          .build());
+    } catch (ResourceNotFoundException e) {
+      LOG.info("Glue lock DynamoDB table <{}> not found, try to create", 
tableName);
+      dynamo.createTable(CreateTableRequest.builder()
+          .tableName(tableName)
+          .keySchema(schema)
+          .attributeDefinitions(definitions)
+          .billingMode(BillingMode.PAY_PER_REQUEST)
+          .build());
+
+      boolean isTableActive = false;
+      while (!isTableActive) {
+        LOG.info("waiting for DynamoDB table <{}> to be active", tableName);
+        try {
+          Thread.sleep(5000);
+        } catch (InterruptedException ie) {
+          LOG.warn("Glue lock DynamoDB table creation sleep interrupted", e);
+        }
+        DescribeTableResponse describeTableResponse = 
dynamo.describeTable(DescribeTableRequest.builder()
+            .tableName(tableName)
+            .build());
+        isTableActive = 
describeTableResponse.table().tableStatus().equals(TableStatus.ACTIVE);
+      }
+    }
+  }
+
+
+  @Override
+  public boolean tryLock(String database, String table, long expireMillis) {
+    String tableId = tableId(database, table);
+
+    Map<String, AttributeValue> key = Maps.newHashMap();
+    key.put(LOCK_TABLE_COL_TABLE_ID, 
AttributeValue.builder().s(tableId).build());
+    key.put(LOCK_TABLE_COL_EXPIRE_TS_MILLIS, AttributeValue.builder().n(
+        Long.toString(System.currentTimeMillis() + expireMillis)
+    ).build());
+
+    Map<String, AttributeValue> expressionValues = Maps.newHashMap();
+    expressionValues.put(":tid", AttributeValue.builder().s(tableId).build());
+    expressionValues.put(":ts", 
AttributeValue.builder().n(Long.toString(System.currentTimeMillis())).build());
+
+    try {
+
+      dynamo.putItem(PutItemRequest.builder()
+          .tableName(awsProperties.glueCatalogLockTable())
+          .item(key)
+          // succeed only if there is no lock, or the lock is there but 
already expired
+          .conditionExpression("attribute_not_exists(" +
+              LOCK_TABLE_COL_TABLE_ID + ") OR (" +
+              LOCK_TABLE_COL_TABLE_ID + " = :tid AND " +
+              LOCK_TABLE_COL_EXPIRE_TS_MILLIS + " < :ts)")
+          .expressionAttributeValues(expressionValues)
+          .build());
+      return true;
+    } catch (Exception e) {
+      // most likely it's ConditionalCheckFailedException, but we will catch 
any exception
+      LOG.debug("Acquiring lock {}.{} failed", database, table, e);

Review comment:
       +1, personally speaking non-`ConditionalCheckFailedException` may even 
worth a `log.error`

##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/DynamoLockManager.java
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws.glue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.aws.AwsClientUtil;
+import org.apache.iceberg.aws.AwsProperties;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import software.amazon.awssdk.services.dynamodb.DynamoDbClient;
+import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition;
+import software.amazon.awssdk.services.dynamodb.model.AttributeValue;
+import software.amazon.awssdk.services.dynamodb.model.BillingMode;
+import software.amazon.awssdk.services.dynamodb.model.CreateTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DeleteItemRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableRequest;
+import software.amazon.awssdk.services.dynamodb.model.DescribeTableResponse;
+import software.amazon.awssdk.services.dynamodb.model.KeySchemaElement;
+import software.amazon.awssdk.services.dynamodb.model.KeyType;
+import software.amazon.awssdk.services.dynamodb.model.PutItemRequest;
+import 
software.amazon.awssdk.services.dynamodb.model.ResourceNotFoundException;
+import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType;
+import software.amazon.awssdk.services.dynamodb.model.TableStatus;
+
+class DynamoLockManager implements LockManager {
+
+  private static final Logger LOG = 
LoggerFactory.getLogger(DynamoLockManager.class);
+
+  private static final String LOCK_TABLE_COL_TABLE_ID = "tableId";
+  private static final String LOCK_TABLE_COL_EXPIRE_TS_MILLIS = 
"expireTimestampMillis";
+
+  private static final List<KeySchemaElement> LOCK_TABLE_SCHEMA = 
Lists.newArrayList(
+      KeySchemaElement.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .keyType(KeyType.HASH)
+          .build()
+  );
+
+  private static final List<AttributeDefinition> LOCK_TABLE_COL_DEFINITIONS = 
Lists.newArrayList(
+      AttributeDefinition.builder()
+          .attributeName(LOCK_TABLE_COL_TABLE_ID)
+          .attributeType(ScalarAttributeType.S)
+          .build()
+  );
+
+  private final DynamoDbClient dynamo;
+  private final AwsProperties awsProperties;
+
+  DynamoLockManager(AwsProperties awsProperties) {
+    this(AwsClientUtil.defaultDynamoClient(), awsProperties);
+  }
+
+  DynamoLockManager(DynamoDbClient dynamo, AwsProperties awsProperties) {
+    this.dynamo = dynamo;
+    this.awsProperties = awsProperties;
+    ensureLockTableExists();
+  }
+
+  private void ensureLockTableExists() {
+    ensureTableExists(awsProperties.glueCatalogLockTable(), LOCK_TABLE_SCHEMA, 
LOCK_TABLE_COL_DEFINITIONS);
+  }
+
+  private void ensureTableExists(String tableName, List<KeySchemaElement> 
schema,
+                                 List<AttributeDefinition> definitions) {
+    try {
+      dynamo.describeTable(DescribeTableRequest.builder()
+          .tableName(tableName)
+          .build());
+    } catch (ResourceNotFoundException e) {
+      LOG.info("Glue lock DynamoDB table <{}> not found, trying to create", 
tableName);
+      dynamo.createTable(CreateTableRequest.builder()
+          .tableName(tableName)
+          .keySchema(schema)
+          .attributeDefinitions(definitions)
+          .billingMode(BillingMode.PAY_PER_REQUEST)
+          .build());
+
+      boolean isTableActive = false;
+      while (!isTableActive) {
+        LOG.info("waiting for DynamoDB table <{}> to be active", tableName);
+        try {
+          Thread.sleep(5000);
+        } catch (InterruptedException ie) {
+          LOG.warn("Glue lock DynamoDB table creation sleep interrupted", e);

Review comment:
       Do we want `Thread.currentThread().interrupt` 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:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to