Re: Evicts persisted deltas from memory (Incomplete).

[veg...@gmail.com]

Hi Soren
Thanks for your comments and explanations. I pushed a new patch that
makes WaveletContainer#requestHistory(),
WaveletContainer#requestTransformedHistory() and
WaveletProvider#getHistory() async. Still need to change
getTransformedDelta(),getTransformedDeltaByEndVersion,
getTransformedDeltaHistory, getAppliedDelta,
getAppliedDeltaByEndVersion, getAppliedDeltaHistory
to return ListenableFeature. Also, need to introduce some kind of
synchronization on the maps with deltas.

http://codereview.waveprotocol.org/615001/diff/1002/1003
File
src/org/waveprotocol/box/server/waveserver/DeltaStoreBasedWaveletState.java
(right):

http://codereview.waveprotocol.org/615001/diff/1002/1003#newcode303
Line 303: List<WaveletDeltaRecord> deltas =
readDeltasInRange(deltasAccess, version, version + 1);
On 2011/09/09 15:17:56, Soren Lassen wrote:
> It's sad if we always have to go to disk. Can we first check if we
happen to
> have it in memory?

Done.

http://codereview.waveprotocol.org/615001/diff/1002/1003#newcode386
Line 386: // Now, that we persisted the deltas, evict them from the
memory.
On 2011/09/09 15:17:56, Soren Lassen wrote:
> I wonder if there is any risk that anyone else tries to access these
maps while
> we delete the deltas here.
Probably. Should we introduce read/write locking similar to
WaveletContainerImpl?

http://codereview.waveprotocol.org/615001/diff/1002/1003#newcode389
Line 389: if (persitedAtVersion.equals(version)) {
On 2011/09/09 15:17:56, Soren Lassen wrote:
> this can't happen, according to the way deltasToPersist is
constructed, right?

> to make sure, you can replace this test with

> Preconditions.checkState(!d.getAppliedAtVersion().equals(version));

> in line 377

Done.

http://codereview.waveprotocol.org/615001/diff/1002/1004
File src/org/waveprotocol/box/server/waveserver/WaveletDeltaRecord.java
(right):

http://codereview.waveprotocol.org/615001/diff/1002/1004#newcode49
Line 49: Preconditions.checkNotNull(applied, "null appliedAtVersion");
On 2011/09/09 15:17:56, Soren Lassen wrote:
> good catch

Done.

http://codereview.waveprotocol.org/615001/diff/1002/1005
File src/org/waveprotocol/box/server/waveserver/WaveletState.java
(right):

http://codereview.waveprotocol.org/615001/diff/1002/1005#newcode92
Line 92: throws PersistenceException;
On 2011/09/09 15:17:56, Soren Lassen wrote:
> I don't think we should make getTransformedDeltaHistory,
getAppliedDelta, and
> getAppliedDeltaHistory blocking but rather by asynchronous, by
returning a
> ListenableFuture or taking a listener callback argument. But it will
require a
> lot of code changes of the callers, so that will be a lot of work.

> Maybe I'm wrong, but I worry that we can't just let all the callers
block,
> unless we audit that none of them are holding locks or stuff that will
get
> things into a jam.
Yeah. I will try to handle this in the next patch set.

http://codereview.waveprotocol.org/615001