And read/write transactions to cache interface #330
Conversation
remove accidental refactor fix checkstyle Make read/write transactions invalid after closing
| } | ||
| } | ||
|
|
||
| @Override public Record read(@Nonnull String key) { | ||
| @Override public RealReadTransaction readTransaction() { | ||
| lock.readLock().lock(); |
There was a problem hiding this comment.
Not sure this is good idea to lock in one place but unlock in RealReadTransaction (via closeRead).
It would be better do it in one place RealReadTransaction so pass lock object to RealReadTransaction
| return cache.read(key); | ||
| } | ||
|
|
||
| @Override public Collection<Record> read(@Nonnull Collection<String> keys) { |
There was a problem hiding this comment.
Should it be more generic like:
<R> R execute(Transactional<Cache, R> action) {
return action.call(cache);
}
Usage:
transaction.execute(new Transactional<>() {
public Collection<Record> call(Cache cache) {
return cache.read(keys);
}
})
Even maybe we can merge Read and Write transactions to one class
|
@sav007 Took a crack at the generic functional approach. Agree that keeping the locking logic in one spot is a lot cleaner! |
| } finally { | ||
| lock.readLock().unlock(); | ||
| } | ||
| @Override public <R> Transaction<ReadableCache, R> readTransaction() { |
There was a problem hiding this comment.
Not sure if it makes sense, but we can simplify API even more, so instead returning the transaction why don't pass Transactional as a parameter here:
@Override public <R> R executeWithReadTransaction(Transactional<ReadableCache, R> transactional) {
try {
lock.readLock().lock();
return transactional.call(RealCache.this);
} finally {
lock.readLock().unlock();
}
}
The same for write. In this case we can rid off one indirection Transaction class at all.
|
Yep - you are 100% right the two levels of indirection is unnecssary |
Closes #327
To better match the general apollo architecture: https://airbnb.quip.com/wsn6AS0cYH84