Cache limits #148
Cache limits #148
Conversation
gfr10598
force-pushed
the
cache-limits
branch
from
6fa8996 to
7fdba83
Compare
Pull Request Test Coverage Report for Build 1336
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1353
💛 - Coveralls |
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager.go, line 88 at r2 (raw file):
am.mutex.Lock() defer am.mutex.Unlock() am.numPending--
decreasing numPending should be done earlier in func that load the dataset
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager.go, line 92 at r2 (raw file):
old, ok := am.annotators[key] if !ok { return ErrGoroutineNotOwner
This should NOT happen. Put a log hear. We should never waste memory & CPU to reload a dataset already in memory.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager.go, line 95 at r2 (raw file):
} if old != nil { return ErrMapEntryAlreadySet
This should NOT happen. Put a log hear. We should never waste memory & CPU to reload a dataset already in memory.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager.go, line 95 at r2 (raw file):
} if old != nil { return ErrMapEntryAlreadySet
This should NOT happen. Put a log here. We should never waste memory & CPU to reload a dataset already in memory.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager.go, line 88 at r2 (raw file):
Previously, yachang wrote…
decreasing numPending should be done earlier in func that load the dataset
Done.
manager/manager.go, line 95 at r2 (raw file):
Previously, yachang wrote…
This should NOT happen. Put a log here. We should never waste memory & CPU to reload a dataset already in memory.
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
handler/handler.go, line 161 at r3 (raw file):
} // AnnotateV2 finds an appropriate Annotator based on the requested Date, and creates a
Find a better name for this func
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
handler/handler.go, line 132 at r3 (raw file):
// TODO move to annotatormanager package soon. // DEPRECATED: This will soon be replaced with AnnotateV2() func AnnotateLegacy(date time.Time, ips []api.RequestData) (map[string]*api.GeoData, time.Time, error) {
Find a better name for this func. "Legacy" was used to describe Maxmind datasets before cutoff date. If reused here, it might cause confusion.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager.go, line 128 at r3 (raw file):
} } return false
Add a note here: If the number of datasets in memory reached max, the first trial to add a new dataset will ALWAYS fail but it will kick out a dataset, so the next trial might succeed.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager.go, line 200 at r3 (raw file):
return ann, nil } if date.Before(geoloader.GeoLite2StartDate) {
Add a TODO to indicate this is a temp hack.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
handler/handler.go, line 132 at r3 (raw file):
Previously, yachang wrote…
Find a better name for this func. "Legacy" was used to describe Maxmind datasets before cutoff date. If reused here, it might cause confusion.
This is temporary code checked in previously. It is labelled DEPRECATED. I don't think this is worth changing in the PR.
handler/handler.go, line 161 at r3 (raw file):
Previously, yachang wrote…
Find a better name for this func
I will create a separate PR for this.
manager/manager.go, line 128 at r3 (raw file):
Previously, yachang wrote…
Add a note here: If the number of datasets in memory reached max, the first trial to add a new dataset will ALWAYS fail but it will kick out a dataset, so the next trial might succeed.
Done.
manager/manager.go, line 200 at r3 (raw file):
Previously, yachang wrote…
Add a TODO to indicate this is a temp hack.
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager.go, line 124 at r4 (raw file):
// If the dataset is not loaded or pending, check: // A: If there are already MaxPending loads in process: // Do nothing and reply with ErrPendingAnnotatorLoad (even though this isn't true)
consider return err here instead of bool?
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager.go, line 124 at r4 (raw file):
Previously, yachang wrote…
consider return err here instead of bool?
Fixed comment instead. The caller doesn't need to know more. The metrics will capture enough data for debugging.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
manager/manager_test.go, line 35 at r4 (raw file):
"Maxmind/2018/01/04/20180401T054119Z-GeoLite2-City-CSV.zip", "Maxmind/2018/01/05/20180501T054119Z-GeoLite2-City-CSV.zip", "Maxmind/2018/01/06/20180601T054119Z-GeoLite2-City-CSV.zip"}
Why you need those many entries? You only use names[0], names[1], names[2]
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 LGTMs obtained
manager/manager_test.go, line 35 at r4 (raw file):
Previously, yachang wrote…
Why you need those many entries? You only use names[0], names[1], names[2]
It uses 0..4. See comment "try to load 2 more"
This adds limits on number of items in the AnnotatorMap, and number of pending loads. The eviction policy is random.
Note that anyone holding an annotator will prevent it from being collected by the GC, so simply evicting it is not a guarantee that the memory will be reclaimed.
I have run the unit test with -race, which gives some confidence that it is probably correct, but is no substitute for careful inspection.
This change is