graphql/batch_executor: rm caching for expensive work units

[jl3329]

Caching was only previously enabled for fields labeled Expensive. The code generating the cache key uses reflect.Comparable to see if the source object for the field can be hashed and used in the cache key.

However, reflect.Comparable cannot determine if the underlying type of an interface is comparable.

This means that a struct with an interface field will panic when used in the cache key if the underlying type of the interface is non-comparable.

e.g.

type Foo interface {
    Bar()
}

type Uncomparable []int

func (u Uncomparable) Bar() {}

type Baz struct {
    Field Foo
}

x := Baz{
    Field: Uncomparable([]int{1,2,3}),
}
// The following returns true. Using `x` in the cache key will panic.
// because the slice field cannot be hashed.
reflect.TypeOf(x).Comparable()

Since caching was only enabled for fields labeled expensive, I think it's reasonable to disable the caching for consistency. Note that for non-websocket use cases, the cache would likely not be used anyways.

If caching turns out to be important enough to reimplement, consider a hashing the sources in a way such that non-comparable fields are handled.

[steveshi7]

What are the performance/load implications of making this change? Is there a way that we can roll this out safely?

[jl3329]

in the context of Samsara, since we do remote execution, i dont think reactive.Cache is ever hit because it caches values in the context object which dies after the request is finished.

The exception is there is a case where some query hits the same fieldfunc twice:

{
 organization(id: 999){
    devices {
      batchFieldFunc
    }
    tags(tagIds: [1,2,3]){
      devices {
        batchFieldFunc
      }
    }
  }
}

but i dont think we should encourage developers to write queries like this.

I don't know how we can feature flag this change but i was planning on using staging to test performance of some expensive field funcs and to make sure things dont break

[coveralls]

Pull Request Test Coverage Report for Build 3385

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• 4 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.01%) to 66.146%

---

Files with Coverage Reduction	New Missed Lines	%
graphql/schemabuilder/input.go	4	60.4%
<!--	Total:	4

Totals
Change from base Build 3371:	-0.01%
Covered Lines:	6016
Relevant Lines:	9095

---

💛 - Coveralls