refactor: Add NewFluxAllocator as an injection point for the GcAllocator #4651
Conversation
This removes GcAllocator from all tests but still provides a way to inject it temporarily for testing.
|
@jsternberg I think I addressed you comments now |
memory/allocator.go
Outdated
|
|
||
| func (a *GcAllocator) TotalAllocated() int64 { | ||
| return a.ResourceAllocator.TotalAllocated() | ||
| func (a *GcAllocator) Allocated() int64 { |
There was a problem hiding this comment.
These functions should be removed I think.
There was a problem hiding this comment.
They are needed for testing that we actually free things.
memory/allocator.go
Outdated
| @@ -248,19 +252,25 @@ func (a *ResourceAllocator) allocator() memory.Allocator { | |||
| } | |||
|
|
|||
| type GcAllocator struct { | |||
| *ResourceAllocator | |||
| mem *ResourceAllocator | |||
There was a problem hiding this comment.
This should just be memory.Allocator. The location that's interested in MaxAllocated() and TotalAllocated() should keep their reference to their ResourceAllocator. GcAllocator doesn't need to know this.
There was a problem hiding this comment.
No, I need a direct reference to ResourceAllocator so I can call Allocated etc in tests to verify that we actually free the allocations.
There was a problem hiding this comment.
You can hold onto both the ResourceAllocator and the GcAllocator to do that. There's also the CheckedAllocator included in arrow for this purpose.
mem := &memory.ResourceAllocator{}
gc := memory.NewGcAllocator(mem)Then you can check mem in the tests.
This removes GcAllocator from all tests but still provides a way to inject it temporarily
for testing.
@jsternberg I think something like this is what you wanted?