----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3508/#review4437 -----------------------------------------------------------
I went through the files in order of the diffs. It wasn't until I got to the end that I realized a lot of this code has simply been moved from one place to another. So some of the comments I made are more applicable to the original code rather than the action of moving the code from one spot to another. I think in generally the refactoring is fine. Just a few comments. This does raise an interesting question as we move forward with copyright statements though. src/org/waveprotocol/box/server/waveserver/MemorySearchProvider.java <https://reviews.apache.org/r/3508/#comment9952> Is this truly a new file, or was the content mostly copied from somewhere else. If it is a new file then I don't think we need the Copyright 2012 Google Inc. statement at the top. src/org/waveprotocol/box/server/waveserver/MemorySearchProvider.java <https://reviews.apache.org/r/3508/#comment9953> Would this be better as an inner class rather than an anonymous class inside a function? src/org/waveprotocol/box/server/waveserver/PerUserWaveViewSubscriber.java <https://reviews.apache.org/r/3508/#comment9954> Again please clarify the copy right statement. src/org/waveprotocol/box/server/waveserver/PerUserWaveViewSubscriber.java <https://reviews.apache.org/r/3508/#comment9956> Could you add a little more here on why this is needed? src/org/waveprotocol/box/server/waveserver/PerUserWaveViewSubscriber.java <https://reviews.apache.org/r/3508/#comment9957> What does the use of the word "computing" mean? src/org/waveprotocol/box/server/waveserver/PerUserWaveViewSubscriber.java <https://reviews.apache.org/r/3508/#comment9955> Remove blank line. src/org/waveprotocol/box/server/waveserver/QueryHelper.java <https://reviews.apache.org/r/3508/#comment9958> Copyright clarification. src/org/waveprotocol/box/server/waveserver/QueryHelper.java <https://reviews.apache.org/r/3508/#comment9959> In what case could the creator not be found. If a creator is not found, is the really an exceptional case? src/org/waveprotocol/box/server/waveserver/QueryHelper.java <https://reviews.apache.org/r/3508/#comment9960> I had to look through the code until the body of the computeLmt() method to figure out LMT = Last Modified Time. I would spell out Last Modified Time (LMT) in the javadoc here. That will make it more obvious. src/org/waveprotocol/box/server/waveserver/QueryHelper.java <https://reviews.apache.org/r/3508/#comment9961> Again possible spell out last modified time (LMT). src/org/waveprotocol/box/server/waveserver/QueryHelper.java <https://reviews.apache.org/r/3508/#comment9962> What is the significance of the term Token here? It is simply called value elsewhere. If we really want to call this a token, then perhaps we should rename the enum member variable name. src/org/waveprotocol/box/server/waveserver/TokenQueryType.java <https://reviews.apache.org/r/3508/#comment9963> Copyright Statement Clarification. src/org/waveprotocol/box/server/waveserver/Wave.java <https://reviews.apache.org/r/3508/#comment9964> This file has no copyright statement. test/org/waveprotocol/box/server/waveserver/MemorySearchProviderTest.java <https://reviews.apache.org/r/3508/#comment9965> Copyright Clarification. - Michael On 2012-01-14 18:06:22, Yuri Zelikov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3508/ > ----------------------------------------------------------- > > (Updated 2012-01-14 18:06:22) > > > Review request for wave, Michael MacFadden and Lennard de Rijk. > > > Summary > ------- > > The goal of this patch is to decouple the search implementation > (SearchProvider) from the waves accessing logic (WaveMap). It should allow > for alternative implementations of search providers - not dependent on > concrete implementation of waves loading and access. The biggest problem with > current implementation is that it requires to load all waves into memory on > the server start up. An alternative (not memory based) search implementation > would allow to load the waves lazily and evict later. > > > This addresses bug WAVE-325. > https://issues.apache.org/jira/browse/WAVE-325 > > > Diffs > ----- > > src/org/waveprotocol/box/server/waveserver/MemorySearchProvider.java > PRE-CREATION > src/org/waveprotocol/box/server/waveserver/PerUserWaveViewSubscriber.java > PRE-CREATION > src/org/waveprotocol/box/server/waveserver/QueryHelper.java PRE-CREATION > src/org/waveprotocol/box/server/waveserver/TokenQueryType.java PRE-CREATION > src/org/waveprotocol/box/server/waveserver/Wave.java PRE-CREATION > src/org/waveprotocol/box/server/waveserver/WaveMap.java 42eb62b > src/org/waveprotocol/box/server/waveserver/WaveServerModule.java 4892dc3 > src/org/waveprotocol/box/server/waveserver/WaveletContainerImpl.java > ca05900 > test/org/waveprotocol/box/server/waveserver/MemorySearchProviderTest.java > PRE-CREATION > > test/org/waveprotocol/box/server/waveserver/PerUserWaveViewSubscriberTest.java > PRE-CREATION > test/org/waveprotocol/box/server/waveserver/WaveDigesterTest.java 9a49ab2 > test/org/waveprotocol/box/server/waveserver/WaveMapTest.java 17ba0df > > Diff: https://reviews.apache.org/r/3508/diff > > > Testing > ------- > > > Thanks, > > Yuri > >
