incubator-wave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yuri Zelikov" <vega...@gmail.com>
Subject Re: Review Request 12725: Assorted federation fixes
Date Thu, 18 Jul 2013 21:09:51 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12725/#review23439
-----------------------------------------------------------



src/org/waveprotocol/box/server/frontend/ClientFrontendImpl.java
<https://reviews.apache.org/r/12725/#comment47315>

    I think that comments in code are not a good practice. If you think that the snippet should
be elaborated, you can extract it into a separate method with descriptive name.



src/org/waveprotocol/box/server/frontend/ClientFrontendImpl.java
<https://reviews.apache.org/r/12725/#comment47316>

    It is a good practice to check if the log level is loggable before actually logging something,
i.e.
    if (LOG.isFineLoggable()){
    LOG.isFineLoggable()
    }



src/org/waveprotocol/box/server/frontend/ClientFrontendImpl.java
<https://reviews.apache.org/r/12725/#comment47317>

    Ditto, remove comment and instead extract a method if needed. Same regarding loggable
checking



src/org/waveprotocol/box/server/frontend/WaveletInfo.java
<https://reviews.apache.org/r/12725/#comment47318>

    ditto



src/org/waveprotocol/box/server/frontend/WaveletInfo.java
<https://reviews.apache.org/r/12725/#comment47319>

    ditto



src/org/waveprotocol/box/server/waveserver/LocalWaveletContainer.java
<https://reviews.apache.org/r/12725/#comment47320>

    Nice fix!



src/org/waveprotocol/box/server/waveserver/MemoryPerUserWaveViewHandlerImpl.java
<https://reviews.apache.org/r/12725/#comment47321>

    ditto



src/org/waveprotocol/box/server/waveserver/PerUserWaveViewDistpatcher.java
<https://reviews.apache.org/r/12725/#comment47322>

    ditto



src/org/waveprotocol/box/server/waveserver/PerUserWaveViewDistpatcher.java
<https://reviews.apache.org/r/12725/#comment47323>

    ditto



src/org/waveprotocol/box/server/waveserver/PerUserWaveViewDistpatcher.java
<https://reviews.apache.org/r/12725/#comment47324>

    ditto



src/org/waveprotocol/box/server/waveserver/RemoteWaveletContainerImpl.java
<https://reviews.apache.org/r/12725/#comment47325>

    Remove comment



src/org/waveprotocol/box/server/waveserver/RemoteWaveletContainerImpl.java
<https://reviews.apache.org/r/12725/#comment47326>

    Better extract the code snippet into a separate method and move the comment into method
javadoc



src/org/waveprotocol/box/server/waveserver/RemoteWaveletContainerImpl.java
<https://reviews.apache.org/r/12725/#comment47327>

    Please remove the comment.



src/org/waveprotocol/box/server/waveserver/RemoteWaveletContainerImpl.java
<https://reviews.apache.org/r/12725/#comment47328>

    Remove comment



src/org/waveprotocol/box/server/waveserver/RemoteWaveletContainerImpl.java
<https://reviews.apache.org/r/12725/#comment47329>

    Use isLoggable



src/org/waveprotocol/box/server/waveserver/RemoteWaveletContainerImpl.java
<https://reviews.apache.org/r/12725/#comment47330>

    Extract to a method, move comment to javadoc



src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java
<https://reviews.apache.org/r/12725/#comment47331>

    Check if loggable



src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java
<https://reviews.apache.org/r/12725/#comment47332>

    Check if loggable



src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java
<https://reviews.apache.org/r/12725/#comment47333>

    Check if loggable



src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java
<https://reviews.apache.org/r/12725/#comment47334>

    Please put a space after TODO
    /TODO(ali) -> //TODO (ali):



src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java
<https://reviews.apache.org/r/12725/#comment47335>

    Check if loggable



src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java
<https://reviews.apache.org/r/12725/#comment47336>

    Please add a space after //



src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java
<https://reviews.apache.org/r/12725/#comment47337>

    Check if loggable



src/org/waveprotocol/box/server/waveserver/Wave.java
<https://reviews.apache.org/r/12725/#comment47338>

    Check if loggable



src/org/waveprotocol/box/server/waveserver/WaveServerImpl.java
<https://reviews.apache.org/r/12725/#comment47340>

    Add space after //



src/org/waveprotocol/box/server/waveserver/WaveServerImpl.java
<https://reviews.apache.org/r/12725/#comment47339>

    Please add your name on TODO



src/org/waveprotocol/box/server/waveserver/WaveServerImpl.java
<https://reviews.apache.org/r/12725/#comment47341>

    Extract to a method and move comment to javadoc



src/org/waveprotocol/box/server/waveserver/WaveletContainerImpl.java
<https://reviews.apache.org/r/12725/#comment47342>

    Please add @Nullable on the method since it can return null.



src/org/waveprotocol/box/webclient/client/WindowTitleHandler.java
<https://reviews.apache.org/r/12725/#comment47344>

    Nice!



src/org/waveprotocol/box/webclient/search/WaveBasedDigest.java
<https://reviews.apache.org/r/12725/#comment47345>

    Nice!



src/org/waveprotocol/wave/client/wavepanel/impl/reader/Reader.java
<https://reviews.apache.org/r/12725/#comment47346>

    Is this a TODO?



src/org/waveprotocol/wave/model/conversation/TitleHelper.java
<https://reviews.apache.org/r/12725/#comment47347>

    Please add your name on TODO



src/org/waveprotocol/wave/model/conversation/TitleHelper.java
<https://reviews.apache.org/r/12725/#comment47348>

    Please put the code after "if" into brackets
    i.e.
    if () {
    return "";
    }



src/org/waveprotocol/wave/model/id/IdGeneratorImpl.java
<https://reviews.apache.org/r/12725/#comment47349>

    Basically "getX" is reserved for the getter of X. I guess this method should start with
"build" or "create"


- Yuri Zelikov


On July 18, 2013, 3:54 p.m., Ali Lown wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12725/
> -----------------------------------------------------------
> 
> (Updated July 18, 2013, 3:54 p.m.)
> 
> 
> Review request for wave, Bruno Gonzalez, Vicente J. Ruiz Jurado, and Yuri Zelikov.
> 
> 
> Repository: wave-git
> 
> 
> Description
> -------
> 
> Federation has been broken for a while again, with assorted shiny's and server-side errors
if somebody attempted to make it work.
> 
> This patch is the squashed form of my 2013-fedfix-test branch, so to see each individual
part it is probably easiest to read the commit log at https://github.com/alown/wave/commits/2013-fedfix-test.
(Rebased against currently trunk).
> 
> This patch results in federation working again.
> It also adds assorted logging (mostly at FINE) level of useful things to enable debugging+fixing
of this section of the codebase in the future.
> 
> 
> Diffs
> -----
> 
>   src/org/waveprotocol/box/server/frontend/ClientFrontendImpl.java 68f70a8 
>   src/org/waveprotocol/box/server/frontend/WaveletInfo.java 76fda42 
>   src/org/waveprotocol/box/server/waveserver/LocalWaveletContainer.java 2d8a891 
>   src/org/waveprotocol/box/server/waveserver/MemoryPerUserWaveViewHandlerImpl.java d995d5e

>   src/org/waveprotocol/box/server/waveserver/PerUserWaveViewDistpatcher.java f4db509

>   src/org/waveprotocol/box/server/waveserver/RemoteWaveletContainerImpl.java a48cfc6

>   src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java 4201531 
>   src/org/waveprotocol/box/server/waveserver/Wave.java 94e7a43 
>   src/org/waveprotocol/box/server/waveserver/WaveServerImpl.java 89e03d1 
>   src/org/waveprotocol/box/server/waveserver/WaveletContainerImpl.java 31fd121 
>   src/org/waveprotocol/box/webclient/client/WindowTitleHandler.java 08987b0 
>   src/org/waveprotocol/box/webclient/search/WaveBasedDigest.java e6137ce 
>   src/org/waveprotocol/wave/client/StageTwo.java 64228ab 
>   src/org/waveprotocol/wave/client/wavepanel/impl/reader/Reader.java bee1733 
>   src/org/waveprotocol/wave/model/conversation/TitleHelper.java ac2c5b3 
>   src/org/waveprotocol/wave/model/id/IdGenerator.java 5c9dec8 
>   src/org/waveprotocol/wave/model/id/IdGeneratorImpl.java 1769f92 
>   src/org/waveprotocol/wave/model/wave/opbased/WaveViewImpl.java a2cff45 
>   test/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImplTest.java 30e0c2d

>   test/org/waveprotocol/wave/concurrencycontrol/wave/CcBasedWaveViewTest.java 06e1cfc

> 
> Diff: https://reviews.apache.org/r/12725/diff/
> 
> 
> Testing
> -------
> 
> Tested by running on eezysys.co.uk and stenyak.com and verifiying.
> Verified that:
> - Local + remote waves can be made
> - Addition of remote participants results in the wave at the remote server
> - Blips can be added to a wave by either local or remote clients
> 
> - Concurrent usage isn't really tested, but then concurrent usage randomly breaks at
the moment anyway.
> 
> 
> Thanks,
> 
> Ali Lown
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message