[GitHub] [iceberg] singhpk234 commented on pull request #4763: AWS : Use strongly consistent read while acquiring lock via DDBLockManager

[GitBox <gi...@apache.org>]

singhpk234 commented on PR #4763:
URL: https://github.com/apache/iceberg/pull/4763#issuecomment-1126640800

   > When acquiring the lock we do conditional puts to validate that no other entity is holding the lock, or in the case where there is an existing lock found in the eventual consistent read, we validate via the conditional put that it's not a different lease id than expected
   
   Yup these validation are there to avoid correctness scenario, but from performance perspective we wasted a GET (reading stale data) & PUT (trying to acquire lock based on stale data). Also in the case we did a GET and found that the record was stale, this includes a sleep of 15 sec ([by default](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/CatalogProperties.java#L67-L68)), before making the PUT which will eventually fail with ConditionalCheckFailedException.
   https://github.com/apache/iceberg/blob/f85366269a37a960bc05a481c41ea1af5df01d83/aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbLockManager.java#L215-L220
   
   > but in those cases the strong consistent read may lead to longer times to acquire the lock
   
   I agree, strong consistent can be less performant than eventual consistent read in terms of latency, I didn't find a public doc bench-marking the two API's, can share a data point though we did an internal benchmark a while back in a high write dynamo table the max we were hurt were in p90 by double digit in ms (though I would take this with a pinch of salt, as it's highly dependent on your workload). Also this is something [DDBCatalog](https://github.com/apache/iceberg/blob/master/aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java#L164) already uses 
   
   > as opposed to maybe failing faster on the put
   
   In case we read a stale entry we don't fail fast via put condition check, we wait for the lease to expire which could be 15 sec by default , this time gap is much much greater than what we could expect our consistent read is gonna add.
   
   > If it's more for reducing the chance of failure on the conditional put, would want to make sure it's a change that existing users for DDB Lock manager
   
   [HadoopCatalog](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java#L113) also uses lockManager so a user using (Hadoop + DDB as lock manager) could certainly benefit.
   
    > considering new users could just use GlueCatalog and optimistic locking
   
   There was interesting discussion a while back on deprecating lockManager requirement for Glue, but from @rdblue's [comment](https://github.com/apache/iceberg/pull/4166#issuecomment-1112636936) looks like we decided to keep this.
   
   All in all I would say, I do see some value in doing this, let me WDYT considering the above. 


-- 
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