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/05/13 12:12:29 UTC
[GitHub] [iceberg] singhpk234 opened a new pull request, #4763: AWS : Use strongly consistent read while acquiring lock via DDBLockManager
singhpk234 opened a new pull request, #4763:
URL: https://github.com/apache/iceberg/pull/4763
At present DDB get request uses eventually consistent reads by default unless specified to use strongly consistent read.
ref : https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.ReadConsistency.html
As per my understanding we should always read most update date data with strong consistency as with eventually consistent read we can read stale data, when read in very short interval of write.
This PR attempts to make GET's of DDB strongly consistent, as by default GET's are eventually consistent.
cc @jackye1995 @rajarshisarkar @amogh-jahagirdar @arminnajafi @xiaoxuandev @yyanyy
--
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] singhpk234 commented on pull request #4763: AWS : Use strongly consistent read while acquiring lock via DDBLockManager
Posted by 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
[GitHub] [iceberg] jackye1995 merged pull request #4763: AWS : Use strongly consistent read while acquiring lock via DDBLockManager
Posted by GitBox <gi...@apache.org>.
jackye1995 merged PR #4763:
URL: https://github.com/apache/iceberg/pull/4763
--
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 #4763: AWS : Use strongly consistent read while acquiring lock via DDBLockManager
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on PR #4763:
URL: https://github.com/apache/iceberg/pull/4763#issuecomment-1126795636
Thanks for the detailed explanation @singhpk234 ! This change makes a lot of sense to me now.
>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.
This is a great point, I think in practice the increase in latency for a strongly consistent read is negligible especially compared to the default 15 second lease time as you pointed out.
This combined with HadoopCatalog + DDB usage, plus I see in AWS's DynamoDBLockClient (also used for distributed locking) https://github.com/awslabs/amazon-dynamodb-lock-client/blob/3f8545a8dff479cb919c305cab05692c6c9464d5/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java#L1285 we also leverage strongly consistent reads.
--
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 #4763: AWS : Use strongly consistent read while acquiring lock via DDBLockManager
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on PR #4763:
URL: https://github.com/apache/iceberg/pull/4763#issuecomment-1126509083
Overall, I think this change should be fine, but I just want to verify a few things to make sure my understanding is correct:
1.) 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. Could we elaborate on the case (time where where an eventual consistent read of the lock will lead to a correctness issue in different entities trying to grab the lock). The case that comes to mind is we read an outdated version and then in the conditional put to acquire the lock we fail to acquire it since the versions are different. Perhaps in streaming workloads where the lock is repeatedly getting acquired and released, there's also a chance that the version is out dated due to eventual consistency; but in those cases the strong consistent read may lead to longer times to acquire the lock as opposed to maybe failing faster on the put? I'll need to think more on the tradeoff here I suppose.
2.) Considering Dynamo locking was originally built due to a lack of optimistic locking support from Glue, I am not sure how many new users would use this locking support and so we may not want to add to it unless there's a correctness issue in the locking (1); if there's a correctness issue, definitely makes sense to fix that.
So in short, if there's a correctness issue in terms of acquiring the lock due to the eventual consistent read, makes sense and would be nice to see an example timeline of systems grabbing the lock.
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 would benefit from in their workload considering new users could just use GlueCatalog and optimistic locking.
--
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