Merge 59dba8d into d25a6fd

broadinstitute · Aug 13, 2020 · 2e2e81f · 2e2e81f
2 parents d25a6fd + 59dba8d
commit 2e2e81f
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 6 deletions.
diff --git a/README.md b/README.md
@@ -48,7 +48,7 @@ Latest SBT dependency: `"org.broadinstitute.dsde.workbench" %% "workbench-metric
 
 Contains utility functions for talking to Google APIs and DAOs for Google PubSub, Google Directory, Google IAM, and Google BigQuery. 
 
-Latest SBT dependency: `"org.broadinstitute.dsde.workbench" %% "workbench-google" % "0.21-2a218f3"`
+Latest SBT dependency: `"org.broadinstitute.dsde.workbench" %% "workbench-google" % "0.21-TRAVIS-REPLACE-ME"`
 
 To depend on the `MockGoogle*` classes, additionally depend on:
 

diff --git a/google/CHANGELOG.md b/google/CHANGELOG.md
@@ -4,7 +4,7 @@ This file documents changes to the `workbench-google` library, including notes o
 
 ## 0.21
 
-SBT dependency: `"org.broadinstitute.dsde.workbench" %% "workbench-google" % "0.21-2a218f3"`
+SBT dependency: `"org.broadinstitute.dsde.workbench" %% "workbench-google" % "0.21-TRAVIS-REPLACE-ME"`
 
 ### Added
 
@@ -22,6 +22,8 @@ SBT dependency: `"org.broadinstitute.dsde.workbench" %% "workbench-google" % "0.
   - These methods now retry 409s, indicating concurrent modifications to the policy. We retry the entire
     read-modify-write operation as recommended by Google.
 - Made `RetryPredicates` handle `null`s more safely
+- Creating a group sometimes returns a 5xx error code and leaves behind a partially created group which caused problems 
+when we retried creation. Changed to delete the partially created group before retrying
 
 ## 0.20
 

diff --git a/google/src/main/scala/org/broadinstitute/dsde/workbench/google/HttpGoogleDirectoryDAO.scala b/google/src/main/scala/org/broadinstitute/dsde/workbench/google/HttpGoogleDirectoryDAO.scala
@@ -9,14 +9,15 @@ import com.google.api.client.googleapis.json.GoogleJsonResponseException
 import com.google.api.client.http.HttpResponseException
 import com.google.api.services.admin.directory.model.{Group, Member, Members}
 import com.google.api.services.admin.directory.{Directory, DirectoryScopes}
-import com.google.api.services.groupssettings.{Groupssettings, GroupssettingsScopes}
 import com.google.api.services.groupssettings.model.{Groups => GroupSettings}
+import com.google.api.services.groupssettings.{Groupssettings, GroupssettingsScopes}
 import org.broadinstitute.dsde.workbench.google.GoogleCredentialModes._
 import org.broadinstitute.dsde.workbench.google.GoogleUtilities.RetryPredicates._
 import org.broadinstitute.dsde.workbench.metrics.GoogleInstrumentedService
 import org.broadinstitute.dsde.workbench.model._
 
 import scala.concurrent.{ExecutionContext, Future}
+import scala.util.Try
 
 /**
  * Created by mbemis on 8/17/17.
@@ -107,9 +108,24 @@ class HttpGoogleDirectoryDAO(appName: String,
     val inserter = groups.insert(group)
 
     for {
-      _ <- retry(when5xx, whenUsageLimited, when404, whenInvalidValueOnBucketCreation, whenNonHttpIOException)(() => {
+      _ <- retryWithRecover(when5xx,
+                            whenUsageLimited,
+                            when404,
+                            whenInvalidValueOnBucketCreation,
+                            whenNonHttpIOException) { () =>
         executeGoogleRequest(inserter)
-      })
+      } {
+        case t: Throwable if when5xx(t) =>
+          // sometimes creating a group errors with a 5xx error and partially creates the group
+          // when this happens some group apis (create, list members and delete group) say the group exists
+          // while others (add member, get details) say the group does not exist.
+          // calling delete before retrying the create should clean all that up
+          logger.debug(
+            s"Creating Google group ${displayName.take(60)} with email ${groupEmail.value} returned a 5xx error. Deleting partially created group and trying again..."
+          )
+          Try(executeGoogleRequest(groups.delete(groupEmail.value)))
+          throw t
+      }
       _ <- groupSettings match {
         case None           => Future.successful(())
         case Some(settings) => new GroupSettingsDAO().updateGroupSettings(groupEmail, settings)
@@ -175,7 +191,9 @@ class HttpGoogleDirectoryDAO(appName: String,
     val getter = directory.groups().get(groupEmail.value)
 
     retryWithRecover(when5xx, whenUsageLimited, when404, whenInvalidValueOnBucketCreation, whenNonHttpIOException)(
-      () => { Option(executeGoogleRequest(getter)) }
+      () => {
+        Option(executeGoogleRequest(getter))
+      }
     ) {
       case e: HttpResponseException if e.getStatusCode == StatusCodes.NotFound.intValue => None
     }
@@ -217,6 +235,7 @@ class HttpGoogleDirectoryDAO(appName: String,
 
   /**
    * recursive because the call to list all members is paginated.
+   *
    * @param fetcher
    * @param accumulated the accumulated Members objects, 1 for each page, the head element is the last prior request
    *                    for easy retrieval. The initial state is Some(Nil). This is what is eventually returned. This