[
https://issues.apache.org/jira/browse/WAVE-325?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13188548#comment-13188548
]
[email protected] commented on WAVE-325:
----------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3508/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-01-14 18:06:22)
bq.
bq.
bq. Review request for wave, Michael MacFadden and Lennard de Rijk.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. 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.
bq.
bq.
bq. This addresses bug WAVE-325.
bq. https://issues.apache.org/jira/browse/WAVE-325
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. src/org/waveprotocol/box/server/waveserver/MemorySearchProvider.java
PRE-CREATION
bq.
src/org/waveprotocol/box/server/waveserver/PerUserWaveViewSubscriber.java
PRE-CREATION
bq. src/org/waveprotocol/box/server/waveserver/QueryHelper.java PRE-CREATION
bq. src/org/waveprotocol/box/server/waveserver/TokenQueryType.java
PRE-CREATION
bq. src/org/waveprotocol/box/server/waveserver/Wave.java PRE-CREATION
bq. src/org/waveprotocol/box/server/waveserver/WaveMap.java 42eb62b
bq. src/org/waveprotocol/box/server/waveserver/WaveServerModule.java 4892dc3
bq. src/org/waveprotocol/box/server/waveserver/WaveletContainerImpl.java
ca05900
bq.
test/org/waveprotocol/box/server/waveserver/MemorySearchProviderTest.java
PRE-CREATION
bq.
test/org/waveprotocol/box/server/waveserver/PerUserWaveViewSubscriberTest.java
PRE-CREATION
bq. test/org/waveprotocol/box/server/waveserver/WaveDigesterTest.java
9a49ab2
bq. test/org/waveprotocol/box/server/waveserver/WaveMapTest.java 17ba0df
bq.
bq. Diff: https://reviews.apache.org/r/3508/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Yuri
bq.
bq.
> Decouple the search implementation from the waves persistence logic.
> --------------------------------------------------------------------
>
> Key: WAVE-325
> URL: https://issues.apache.org/jira/browse/WAVE-325
> Project: Wave
> Issue Type: Improvement
> Components: Server
> Reporter: Yuri Zelikov
> Assignee: Yuri Zelikov
>
> 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 message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
