Skip to content

Commit

Permalink
fix(GraphQL): Fix flaky tests caused by multiple increments of schema…
Browse files Browse the repository at this point in the history
…UpdateCounter. (#7489)

Fixes GRAPHQL-1050
There were a couple of tests that were behaving flakily because for a single schema update we were incrementing schemaUpdateCounter multiple times, in case the schema update was for a non-Galaxy namespace. This behavior was caused by a bug in schema lazy loading which got added recently with multi-tenancy in PR #7400.

Consider a scenario when we don't have any schema stored in the admin GraphQL server for a non-Galaxy namespace.
Sending a schema POST request at /admin/schema gives a badger subscription update for this schema. Since we don't have any schema in the admin GraphQL server, we update the schemaUpdateCounter but don't update the schema in both the admin and the main GraphQL server, because this schema should be loaded lazily when the main GraphQL server receives a GraphQL request.

Now, what happens is, badger gives another update from the subscription, which contains the same schema with a higher version. Since the admin server didn't store any information about this schema previously, so it is not able to decide whether to reject this update. Resulting in the schemaUpdateCounter being incremented again. Yet, nothing stored in the admin server about the schema.

This results in the schemaUpdateCounter being incremented multiple times for a single schema update request.
This behavior persists until the main GraphQL server doesn't get a request for that namespace resulting in the lazy loading of the schema, which both stores the schema in the admin server and loads it into the main server. Once that happens, updating schema once would only update the schemaUpdateCounter once as well.

So, to fix this bug we introduced a new variable loaded in gqlSchema struct. This flag is set to true whenever a schema is loaded in the main GraphQL server. If there is no schema stored in the admin server or if this flag is false and we get an update from badger subscription, then we just store the gqlSchema on the admin server with this flag set to false, but don't load it into the main GraphQL server. Only if this flag were true, then the schema would be loaded in the main GraphQL server. This mechanism lets the admin server know correctly when to increment the schemaUpdateCounter.
  • Loading branch information
JatinDev543 authored Feb 26, 2021
1 parent 97f280f commit c1d74c7
Showing 1 changed file with 12 additions and 4 deletions.
16 changes: 12 additions & 4 deletions graphql/admin/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ type gqlSchema struct {
Schema string `json:"schema,omitempty"`
Version uint64
GeneratedSchema string
loaded bool // This indicate whether the schema has been loaded into graphql server or not
}

type adminServer struct {
Expand Down Expand Up @@ -571,10 +572,16 @@ func newAdminResolver(

server.incrementSchemaUpdateCounter(ns)
// if the schema hasn't been loaded yet, then we don't need to load it here
if !ok {
glog.Info("Skipping in-memory GraphQL schema update, it will be lazy-loaded later.")
currentSchema, ok = server.schema[ns]
if !(ok && currentSchema.loaded) {
// this just set schema in admin server, so that next invalid badger subscription update gets rejected upfront
server.schema[ns] = newSchema
glog.Infof("Skipping in-memory GraphQL schema update, it will be lazy-loaded later.")
return
}

// update this schema in both admin and graphql server
newSchema.loaded = true
server.schema[ns] = newSchema
server.resetSchema(ns, gqlSchema)

Expand Down Expand Up @@ -689,7 +696,7 @@ func (as *adminServer) initServer() {
glog.Infof("Error reading GraphQL schema: %s.", err)
continue
}

sch.loaded = true
as.schema[x.GalaxyNamespace] = sch
// adding the actual resolvers for updateGQLSchema and getGQLSchema only after server has
// current GraphQL schema, if there was any.
Expand Down Expand Up @@ -848,7 +855,7 @@ func (as *adminServer) resetSchema(ns uint64, gqlSchema schema.Schema) {
func (as *adminServer) lazyLoadSchema(namespace uint64) {
// if the schema is already in memory, no need to fetch it from disk
as.mux.RLock()
if _, ok := as.schema[namespace]; ok {
if currentSchema, ok := as.schema[namespace]; ok && currentSchema.loaded {
as.mux.RUnlock()
return
}
Expand All @@ -874,6 +881,7 @@ func (as *adminServer) lazyLoadSchema(namespace uint64) {

as.mux.Lock()
defer as.mux.Unlock()
sch.loaded = true
as.schema[namespace] = sch
as.resetSchema(namespace, generatedSchema)

Expand Down

0 comments on commit c1d74c7

Please sign in to comment.