-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ID-1327 revert synchronous group removals and refactor #1480
ID-1327 revert synchronous group removals and refactor #1480
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you retain the shape of the response to what it was before
@Ghost-in-a-Jar done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I left a few suggestions for test improvement.
Now that google group synchronization is fixed, its no longer needed.
I'm probably out of the loop here, is there somewhere I can go to learn more about how Google Group synchronization was fixed (ex. an incident review page)?
src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala
Outdated
Show resolved
Hide resolved
src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
Ticket: https://broadworkbench.atlassian.net/browse/ID-1327
Reverting the synchronous group sync for policy removals. This was a band-aid, but has the potential to create long request latencies. Now that google group synchronization is fixed, its no longer needed.
Also did a little refactor of the sync de-duplication. Instead of relying on an exception for control flow, we now rely on an
Option
. This is best-practice, since using exceptions for control flow incurs a significant performance penalty in JVM languages.PR checklist