----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7958/#review13539 -----------------------------------------------------------
Some more comments. Andrew, the patch is really big one! I really appreciate your efforts, thanks! ./README.export-import <https://reviews.apache.org/r/7958/#comment29024> Can you please provide some more details on how to export/import the waves? I mean step-by-step instructions like we had for export/import from GWave. ./README.export-import <https://reviews.apache.org/r/7958/#comment29040> transform -> transfer ./README.export-import <https://reviews.apache.org/r/7958/#comment29026> Trailing space ./build.xml <https://reviews.apache.org/r/7958/#comment29025> Why to remove this line? ./src/com/google/wave/api/OperationQueue.java <https://reviews.apache.org/r/7958/#comment29027> Export -> Exports ./src/com/google/wave/api/OperationQueue.java <https://reviews.apache.org/r/7958/#comment29028> Export -> Exports ./src/com/google/wave/api/OperationQueue.java <https://reviews.apache.org/r/7958/#comment29029> Import -> Imports ./src/com/google/wave/api/OperationQueue.java <https://reviews.apache.org/r/7958/#comment29030> Import -> Imports ./src/com/google/wave/api/OperationType.java <https://reviews.apache.org/r/7958/#comment29031> Trailing space ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29032> Can we check here if the log messages og level FINE are loggable before writing to the log? i.e. if (LOG.isLoggable(Level.FINE)) { LOG.fine("Signature base string: " + OAuthSignatureMethod.getBaseString(message)); } ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29033> Inline the return message, there's no need for the "snapshot" variable. Also. do we really need the @SuppressWarnings("unchecked") annotation here? ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29034> Export ->Exports ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29041> Please extract the code related to this comment into a separate method. ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29035> Please inline + remove @SuppressWarnings("unchecked") ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29036> Export -> Exports ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29037> The return type is AttachmentData, not deltas history ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29038> Please extract this code into a method with descriptive name. ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29039> Inline + remove annotation ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29042> Import -> imports ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29043> Extract method ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29044> Inline + annotation ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29045> Import -> Imports ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29046> Same, extract + inline + annotation ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29047> Log.fine -> if (LOG.isLoggable(FINE)) { .... } ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29048> Check if loggable ./src/com/google/wave/api/WaveService.java <https://reviews.apache.org/r/7958/#comment29049> Check if loggable ./src/com/google/wave/api/impl/RawDeltas.java <https://reviews.apache.org/r/7958/#comment29050> Trailing space ./src/org/waveprotocol/box/expimp/Console.java <https://reviews.apache.org/r/7958/#comment29051> Trailing space ./src/org/waveprotocol/box/expimp/Console.java <https://reviews.apache.org/r/7958/#comment29052> Trailing space ./src/org/waveprotocol/box/expimp/DeltaParser.java <https://reviews.apache.org/r/7958/#comment29056> Can we add private constructor to this util class to prevent instantiation? ./src/org/waveprotocol/box/expimp/DeltaParser.java <https://reviews.apache.org/r/7958/#comment29053> static public -> public static ./src/org/waveprotocol/box/expimp/DeltaParser.java <https://reviews.apache.org/r/7958/#comment29054> static public -> public static ./src/org/waveprotocol/box/expimp/DeltaParser.java <https://reviews.apache.org/r/7958/#comment29055> The method inserts to Set<AttachmentId> ids which is an argument to this method. I think it would be better not to modify arguments, instead we can create a set inside the method and return it. ./src/org/waveprotocol/box/expimp/DeltaParser.java <https://reviews.apache.org/r/7958/#comment29066> Can you please add unit test for this method? ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29057> Missing license ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29058> Convert export data to new domain. -> Converts domain names in exported data ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29065> Please add unit test ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29059> Replace -> replaces ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29060> Replace domain in the delta. -> Replaces domain in deltas for Add/Remove operations ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29061> deserialize of participant error -> if there is a problem with deserialization of participant id ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29062> Convert -> Converts ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29067> initAddParticipantOperation - > Why the method starts with "init" ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29068> Convert - > Converts Why init? ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29069> Replaces ./src/org/waveprotocol/box/expimp/DomainConverter.java <https://reviews.apache.org/r/7958/#comment29070> Can we extract the address part without parsing the participantId? Like ParticipantId.of(participant).getAddress() ./src/org/waveprotocol/box/expimp/OAuth.java <https://reviews.apache.org/r/7958/#comment29071> Trailing space ./src/org/waveprotocol/box/expimp/OAuth.java <https://reviews.apache.org/r/7958/#comment29072> Trailing space ./src/org/waveprotocol/box/expimp/OAuth.java <https://reviews.apache.org/r/7958/#comment29073> Trailing space ./src/org/waveprotocol/box/expimp/OAuth.java <https://reviews.apache.org/r/7958/#comment29074> Trailing space ./src/org/waveprotocol/box/expimp/OAuth.java <https://reviews.apache.org/r/7958/#comment29075> Trailing space ./src/org/waveprotocol/box/expimp/WaveExport.java <https://reviews.apache.org/r/7958/#comment29076> Can we use constant instead of "0" ./src/org/waveprotocol/box/expimp/WaveExport.java <https://reviews.apache.org/r/7958/#comment29077> static public -> public static Also, please move the static method above non static methods ./src/org/waveprotocol/box/expimp/WaveExport.java <https://reviews.apache.org/r/7958/#comment29078> Can we use constant instead of 0? ./src/org/waveprotocol/box/expimp/WaveImport.java <https://reviews.apache.org/r/7958/#comment29079> Why not static? ./src/org/waveprotocol/box/expimp/WaveImport.java <https://reviews.apache.org/r/7958/#comment29080> Use constant for api.setFetchFimeout(0) ./src/org/waveprotocol/box/expimp/WaveImport.java <https://reviews.apache.org/r/7958/#comment29081> Import -> Imports ./src/org/waveprotocol/box/expimp/WaveImport.java <https://reviews.apache.org/r/7958/#comment29082> Add spaces around "=" ./src/org/waveprotocol/box/server/robots/OperationContext.java <https://reviews.apache.org/r/7958/#comment29083> iff = "if and only if" Did you change this intentionally? ./src/org/waveprotocol/box/server/robots/OperationContext.java <https://reviews.apache.org/r/7958/#comment29084> Same here ./src/org/waveprotocol/box/server/robots/OperationContext.java <https://reviews.apache.org/r/7958/#comment29085> Can we make the method name more descriptive? Like if the method returns only wavelets that are visible - please include this in the method name. ./src/org/waveprotocol/box/server/robots/OperationContext.java <https://reviews.apache.org/r/7958/#comment29086> Remove space before "long timeout" ./src/org/waveprotocol/box/server/robots/OperationContextImpl.java <https://reviews.apache.org/r/7958/#comment29088> Add @Nullable annotation to the method since it returns null in some cases. ./src/org/waveprotocol/box/server/robots/OperationContextImpl.java <https://reviews.apache.org/r/7958/#comment29087> Remove space before "long timeout" ./src/org/waveprotocol/box/server/robots/OperationContextImpl.java <https://reviews.apache.org/r/7958/#comment29089> Why null and not "ImmutableList.of();" ./src/org/waveprotocol/box/server/robots/operations/ExportDeltasService.java <https://reviews.apache.org/r/7958/#comment29090> Trailing space ./src/org/waveprotocol/box/server/waveserver/DeltaStoreBasedWaveletState.java <https://reviews.apache.org/r/7958/#comment29091> The code for this method is almost identical with difference that is takes timeout argument. Can we modify existing method to call this one with some big value of timeout, so we can avoid code duplication? ./src/org/waveprotocol/box/server/waveserver/DeltaStoreBasedWaveletState.java <https://reviews.apache.org/r/7958/#comment29092> Same, need to avoid code duplication ./src/org/waveprotocol/box/server/waveserver/WaveServerImpl.java <https://reviews.apache.org/r/7958/#comment29093> Same, avoid code duplication ./src/org/waveprotocol/box/server/waveserver/WaveletContainerImpl.java <https://reviews.apache.org/r/7958/#comment29094> Avoid code duplication ./src/org/waveprotocol/box/server/waveserver/WaveletProvider.java <https://reviews.apache.org/r/7958/#comment29095> Retrive -> retrieves - Yuri Zelikov On Nov. 12, 2012, 9:26 a.m., Andrew Kaplanov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7958/ > ----------------------------------------------------------- > > (Updated Nov. 12, 2012, 9:26 a.m.) > > > Review request for wave and Yuri Zelikov. > > > Description > ------- > > Export/Import client utilities and support on the server. > Replaces utilities for export from GWave and import to Wiab. > Released on DataAPI and RobotAPI. > See README.export-import for tutorial. > > > Diffs > ----- > > ./README.export-import PRE-CREATION > ./README.import 1406525 > ./build.properties 1406525 > ./build.xml 1406525 > ./run-export.sh 1406525 > ./run-import.sh 1406525 > ./server-config.xml 1406525 > ./server.config.example 1406525 > ./src/com/google/wave/api/JsonRpcConstant.java 1406525 > ./src/com/google/wave/api/OperationQueue.java 1406525 > ./src/com/google/wave/api/OperationType.java 1406525 > ./src/com/google/wave/api/WaveService.java 1406525 > ./src/com/google/wave/api/event/WaveletFetchedEvent.java 1406525 > ./src/com/google/wave/api/impl/AttachmentData.java PRE-CREATION > ./src/com/google/wave/api/impl/GsonFactory.java 1406525 > ./src/com/google/wave/api/impl/JsonRpcResponseGsonAdaptor.java 1406525 > ./src/com/google/wave/api/impl/RawDeltas.java PRE-CREATION > ./src/org/waveprotocol/box/expimp/Console.java PRE-CREATION > ./src/org/waveprotocol/box/expimp/DeltaParser.java PRE-CREATION > ./src/org/waveprotocol/box/expimp/DomainConverter.java PRE-CREATION > ./src/org/waveprotocol/box/expimp/FileNames.java PRE-CREATION > ./src/org/waveprotocol/box/expimp/OAuth.java PRE-CREATION > ./src/org/waveprotocol/box/expimp/WaveExport.java PRE-CREATION > ./src/org/waveprotocol/box/expimp/WaveImport.java PRE-CREATION > ./src/org/waveprotocol/box/server/CoreSettings.java 1406525 > ./src/org/waveprotocol/box/server/ServerMain.java 1406525 > ./src/org/waveprotocol/box/server/gxp/OAuthAuthorizationCodePage.gxp > PRE-CREATION > ./src/org/waveprotocol/box/server/gxp/OAuthAuthorizeTokenPage.gxp 1406525 > ./src/org/waveprotocol/box/server/robots/OperationContext.java 1406525 > ./src/org/waveprotocol/box/server/robots/OperationContextImpl.java 1406525 > ./src/org/waveprotocol/box/server/robots/RobotApiModule.java 1406525 > > ./src/org/waveprotocol/box/server/robots/active/ActiveApiOperationServiceRegistry.java > 1406525 > ./src/org/waveprotocol/box/server/robots/dataapi/DataApiOAuthServlet.java > 1406525 > > ./src/org/waveprotocol/box/server/robots/dataapi/DataApiOperationServiceRegistry.java > 1406525 > > ./src/org/waveprotocol/box/server/robots/operations/ExportAttachmentService.java > PRE-CREATION > > ./src/org/waveprotocol/box/server/robots/operations/ExportDeltasService.java > PRE-CREATION > > ./src/org/waveprotocol/box/server/robots/operations/ExportSnapshotService.java > PRE-CREATION > ./src/org/waveprotocol/box/server/robots/operations/FetchWaveService.java > 1406525 > > ./src/org/waveprotocol/box/server/robots/operations/ImportAttachmentService.java > PRE-CREATION > > ./src/org/waveprotocol/box/server/robots/operations/ImportDeltasService.java > PRE-CREATION > > ./src/org/waveprotocol/box/server/waveserver/DeltaStoreBasedWaveletState.java > 1406525 > ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java 1406525 > ./src/org/waveprotocol/box/server/waveserver/WaveServerImpl.java 1406525 > ./src/org/waveprotocol/box/server/waveserver/WaveletContainer.java 1406525 > ./src/org/waveprotocol/box/server/waveserver/WaveletContainerImpl.java > 1406525 > ./src/org/waveprotocol/box/server/waveserver/WaveletProvider.java 1406525 > ./src/org/waveprotocol/box/server/waveserver/WaveletState.java 1406525 > ./src/org/waveprotocol/box/waveimport/WaveExport.java 1406525 > ./src/org/waveprotocol/box/waveimport/WaveImport.java 1406525 > ./src/org/waveprotocol/box/waveimport/google/RobotApi.java 1406525 > ./src/org/waveprotocol/box/waveimport/google/RobotSearchDigest.java 1406525 > ./src/org/waveprotocol/box/waveimport/google/RobotSearchDigestGsonImpl.java > 1406525 > ./src/org/waveprotocol/box/waveimport/google/RobotSearchDigestImpl.java > 1406525 > ./src/org/waveprotocol/box/waveimport/google/RobotSearchDigestUtil.java > 1406525 > > ./src/org/waveprotocol/box/waveimport/google/oauth/NeedNewOAuthTokenException.java > 1406525 > ./src/org/waveprotocol/box/waveimport/google/oauth/OAuthCredentials.java > 1406525 > ./src/org/waveprotocol/box/waveimport/google/oauth/OAuthRequestHelper.java > 1406525 > ./src/org/waveprotocol/box/waveimport/google/oauth/OAuthedFetchService.java > 1406525 > ./src/org/waveprotocol/box/waveimport/google/oauth/StableUserId.java > 1406525 > ./src/org/waveprotocol/box/waveimport/google/oauth/UserContext.java 1406525 > ./src/org/waveprotocol/wave/model/image/ImageConstants.java PRE-CREATION > ./test/org/waveprotocol/box/expimp/DomainConverterTest.java PRE-CREATION > > ./test/org/waveprotocol/box/server/robots/dataapi/DataApiOAuthServletTest.java > 1406525 > ./test/org/waveprotocol/box/server/rpc/WaveletProviderStub.java 1406525 > ./test/org/waveprotocol/box/server/waveserver/ImportServletTest.java > 1406525 > ./third_party/runtime/google-api-client/COPYING 1406525 > ./third_party/runtime/google-api-client/README 1406525 > ./third_party/runtime/google-api-client/google-api-client-1.5.0-beta.jar > 1406525 > ./third_party/runtime/google-api-client/google-http-client-1.5.0-beta.jar > 1406525 > ./third_party/runtime/google-api-client/google-oauth-client-1.5.0-beta.jar > 1406525 > > Diff: https://reviews.apache.org/r/7958/diff/ > > > Testing > ------- > > > Thanks, > > Andrew Kaplanov > >
