google-cloud-dotnet icon indicating copy to clipboard operation
google-cloud-dotnet copied to clipboard

Published 20 hours ago • verified publisher icon googleapis

feat: add support for FGAC spanner

Open hemanshv opened this issue 2 years ago • 13 comments

Hi @jskeet and @amanda-tarafa ,

This is a prototype PR for Spanner FGAC with the approach suggested by Amanda. Its still work in progress. Please have a look and provide your valuable feedback. Thanks,

hemanshv avatar Aug 17 '22 05:08 hemanshv

I'll take a look when I'm back, or maybe before if I can.

amanda-tarafa avatar Aug 17 '22 07:08 amanda-tarafa

To what extent is this ready for an actual full review? I'd rather not do several rounds of review when it's not expected to be ready to merge.

jskeet avatar Aug 23 '22 08:08 jskeet

Hey @jskeet @amanda-tarafa, I have updated the PR with the latest changes for FGAC. However I just saw that some tests are failing. I am looking into this. Please do not review will let you know once this fixed.

Thanks,

hemanshv avatar Aug 30 '22 07:08 hemanshv

@hemanshv Please also make sure that you have run all integration tests locally and that they are all green before sending the PR for review. Thanks!

amanda-tarafa avatar Aug 30 '22 08:08 amanda-tarafa

Hey @jskeet @amanda-tarafa The issue with the Integration tests are now fixed. Please review the PR and provide your valuable feedback. Thanks,

hemanshv avatar Sep 01 '22 11:09 hemanshv

Hey @amanda-tarafa , Thanks for the feedback, will work on the comments and update the PR. Thanks,

hemanshv avatar Sep 02 '22 12:09 hemanshv

Hey @amanda-tarafa I have addressed the review comments. Please have a look and provide your feedback. Thanks,

hemanshv avatar Sep 06 '22 13:09 hemanshv

Hey @amanda-tarafa , I have addressed all the review comment apart from the ones related to stats deprecation in commit 2f51daf. Will address the changes related to deprecation in the next commit. Please have a look and provide your feedback.

Thanks.

hemanshv avatar Sep 14 '22 12:09 hemanshv

@hemanshv I'll look again when both commits are in, unless there's something in particular you want me to look at first. Is that OK?

amanda-tarafa avatar Sep 14 '22 14:09 amanda-tarafa

Yes @amanda-tarafa thats fine Thanks

hemanshv avatar Sep 14 '22 14:09 hemanshv

Hey @amanda-tarafa,

I have updated the PR with both the commits. Please have a look and provide your feedback.

hemanshv avatar Sep 15 '22 15:09 hemanshv

Thanks @amanda-tarafa for the review. Will update it and let you know.

hemanshv avatar Sep 20 '22 11:09 hemanshv

Hey @amanda-tarafa , I have updated the PR, please have a look and provide your feedback. Thanks,

hemanshv avatar Sep 22 '22 14:09 hemanshv

I'll look tomorrow or Thursday morning - if @hemanshv is able to fix these before I review, that would avoid any duplication too :)

jskeet avatar Oct 04 '22 14:10 jskeet

Can this PR now be marked "ready for review" if we think it's ready to merge on successful review?

jskeet avatar Oct 07 '22 10:10 jskeet