----------------------------------------------------------- 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 > >
