Impression seenAt #124
Impression seenAt #124
Conversation
Development
…a-client into improvement/impressionSeenAt
patricioe
left a comment
patricioe
left a comment
There was a problem hiding this comment.
Reminder to update the CHANGES file with its description
| private final Cache<Long, Long> _cache; | ||
|
|
||
| public ImpressionObserver(long size) { | ||
| _cache = CacheBuilder.newBuilder() |
There was a problem hiding this comment.
do we need to set the concurrency level?
There was a problem hiding this comment.
how about the expiration settings?
There was a problem hiding this comment.
Regarding cuncurrency level: the cache is only used from the impressions manager which is executed within a singleThreadedExecutor, so we should not have race conditions here, nor benefit from partitioning to minimize contention. I can however set the default value (4) explicitly, in case it changes sometime, or we decide to go multithreaded here
Regarding expiration settings: We discussed this thoroughly with nico, and in order to minimize memory usage, we decided not to attach an extra TTL to each cache item, since the difference between sending null or sending an old timestamp doesn't really make a difference. We let the events server decide whether the timestamp is recent enough to filter the impression from the exp pipeline or not
|
|
||
| Long hash = ImpressionHasher.process(impression); | ||
|
|
||
| synchronized(this) { |
There was a problem hiding this comment.
this can't be here. It'll be too slow. Better to use the compute method from guava cache.
There was a problem hiding this comment.
I guess the caller is single thread correct?
There was a problem hiding this comment.
I'm not familiar with compute method, i'll look into it.
It's single threaded, but I added the synchronized block, in order to avoid surprises in the future, are those statements really that slow? the hashing computation is done outside of the critical region
3 participants
No description provided.