----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32893/#review79166 -----------------------------------------------------------
Looks generally fine to me. You seem to have made other small changes to many of the files (tidying up imports, sanitizing parameters/exception, etc.) most of which are fine. Eagerly sanitizing the file on startup is quite important, how best do we go about getting that? Should we have some may of migrating the existing *.conf files for people, or do we just put that this has changed in big letters in RELEASE-NOTES? I have highlighted a few things I spotted below: server-config.xml <https://reviews.apache.org/r/32893/#comment128354> Why does this need to remain? Actually, why does this file exist at all anymore? We could move the prosody-config section back into build.xml now. server-config.xml <https://reviews.apache.org/r/32893/#comment128353> Can't we get rid of these, given the use of the federation.* config space. test/org/waveprotocol/wave/federation/xmpp/XmppDiscoTest.java <https://reviews.apache.org/r/32893/#comment128352> This may prevent federation between older versions of the server and ones running with this change. Whilst it should occur - as we really shouldn't have anything saying 'google' left in here, I am not convinced this is the right patch... - Ali Lown On April 6, 2015, 7:23 p.m., Yuri Zelikov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32893/ > ----------------------------------------------------------- > > (Updated April 6, 2015, 7:23 p.m.) > > > Review request for wave, Andrew Kaplanov and Ali Lown. > > > Bugs: WAVE-423 > https://issues.apache.org/jira/browse/WAVE-423 > > > Repository: wave > > > Description > ------- > > As of now Wiab uses a custom written configuration framework. The > configuration is defined in server.config and federation.server.config files. > In short - in order to add a new configuration property - one needs to edit 3 > files in several locations. Also, it makes it really inconvenient to override > default settings with custom ones. The introduction of Typesafe Config solves > almost all these issues. Now, all default settings are defined in > reference.conf in HOCON format (typesafe config can also parse properties and > json formats). One can provide application.conf file with settings that will > override those in reference.conf. Or alternatively to pass them as JVM args > or environment variables - which will override both application.conf and > reference.conf. > Currently the only disadvantage is that configuration is not validated > eagerly on startup, but only when accessed. This can be added later. > > > Diffs > ----- > > .classpath 824e720 > .gitignore 42c8e03 > build.xml 52000a0 > run-server.bat 78c9fbf > run-server.sh 3ee3eb4 > server-config.xml 30b33c2 > server.config.example bc25193 > server.federation.config de69730 > server.federation.config.example f0c8d82 > src/org/waveprotocol/box/server/CoreSettings.java 5fbd345 > src/org/waveprotocol/box/server/DataMigrationTool.java 32e0d50 > src/org/waveprotocol/box/server/SearchModule.java 9848a7f > src/org/waveprotocol/box/server/ServerMain.java 94ee5ae > src/org/waveprotocol/box/server/ServerModule.java 0266942 > src/org/waveprotocol/box/server/StatModule.java 6c5af5e > src/org/waveprotocol/box/server/executor/ExecutorsModule.java ca0d365 > src/org/waveprotocol/box/server/persistence/PersistenceModule.java 3016671 > src/org/waveprotocol/box/server/persistence/file/FileAccountStore.java > d4608dc > src/org/waveprotocol/box/server/persistence/file/FileAttachmentStore.java > 96b23a3 > src/org/waveprotocol/box/server/persistence/file/FileDeltaStore.java > a615693 > src/org/waveprotocol/box/server/persistence/file/FileSignerInfoStore.java > e8686f4 > src/org/waveprotocol/box/server/persistence/lucene/FSIndexDirectory.java > 4badd7c > src/org/waveprotocol/box/server/robots/ProfileFetcherModule.java be8f7ea > src/org/waveprotocol/box/server/robots/RobotApiModule.java 6bcaadb > src/org/waveprotocol/box/server/robots/RobotRegistrationServlet.java > b99d274 > src/org/waveprotocol/box/server/robots/agent/AbstractBaseRobotAgent.java > a549cc0 > src/org/waveprotocol/box/server/robots/agent/AbstractCliRobotAgent.java > dd878bc > src/org/waveprotocol/box/server/robots/agent/passwd/PasswordAdminRobot.java > 49dbbf2 > > src/org/waveprotocol/box/server/robots/agent/registration/RegistrationRobot.java > 11fad95 > src/org/waveprotocol/box/server/robots/agent/welcome/WelcomeRobot.java > ba041cd > > src/org/waveprotocol/box/server/robots/operations/GravatarProfilesFetcher.java > 75501a7 > src/org/waveprotocol/box/server/robots/operations/ImportDeltasService.java > c313f12 > src/org/waveprotocol/box/server/rpc/AttachmentServlet.java ce30ac0 > src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java 49d5964 > src/org/waveprotocol/box/server/rpc/ServerRpcProvider.java deaf01b > src/org/waveprotocol/box/server/rpc/UserRegistrationServlet.java 0b858a9 > src/org/waveprotocol/box/server/rpc/WaveClientServlet.java 37bb8de > src/org/waveprotocol/box/server/waveserver/CertificateManagerImpl.java > 75569bc > > src/org/waveprotocol/box/server/waveserver/LucenePerUserWaveViewHandlerImpl.java > 02aa58b > src/org/waveprotocol/box/server/waveserver/NonSigningSignatureHandler.java > e87b6bf > src/org/waveprotocol/box/server/waveserver/SigningSignatureHandler.java > 7133808 > src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java > 9884daf > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java > a46dc93 > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java 5403b5f > src/org/waveprotocol/box/server/waveserver/WaveMap.java 09ee788 > src/org/waveprotocol/box/server/waveserver/WaveServerModule.java 5743594 > src/org/waveprotocol/wave/federation/FederationSettings.java bad3199 > src/org/waveprotocol/wave/federation/xmpp/ComponentPacketTransport.java > bd7b06d > src/org/waveprotocol/wave/federation/xmpp/RemoteDisco.java 1ea43c5 > src/org/waveprotocol/wave/federation/xmpp/XmppDisco.java 79148f5 > src/org/waveprotocol/wave/federation/xmpp/XmppFederationHost.java 7194584 > src/org/waveprotocol/wave/federation/xmpp/XmppFederationHostForDomain.java > 2b077f3 > src/org/waveprotocol/wave/federation/xmpp/XmppFederationRemote.java c2d8a55 > src/org/waveprotocol/wave/federation/xmpp/XmppManager.java 4dad3b6 > src/org/waveprotocol/wave/util/settings/Setting.java 6fad10f > src/org/waveprotocol/wave/util/settings/SettingsBinder.java 0316520 > test/org/waveprotocol/box/server/persistence/file/AccountStoreTest.java > 26b3d8d > test/org/waveprotocol/box/server/persistence/file/AttachmentStoreTest.java > 023d3c9 > test/org/waveprotocol/box/server/persistence/file/CertPathStoreTest.java > 36f67f2 > test/org/waveprotocol/box/server/persistence/file/DeltaStoreTest.java > 6c01f70 > test/org/waveprotocol/box/server/robots/agent/AbstractRobotAgentTest.java > 5f78d04 > test/org/waveprotocol/box/server/rpc/AuthenticationServletTest.java 2e39d2d > test/org/waveprotocol/box/server/rpc/RpcTest.java 8af1078 > test/org/waveprotocol/box/server/rpc/UserRegistrationServletTest.java > bd83db8 > test/org/waveprotocol/box/server/waveserver/CertificateManagerImplTest.java > 75ac795 > > test/org/waveprotocol/box/server/waveserver/LucenePerUserWaveViewProviderTest.java > 078203c > > test/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImplTest.java > d966305 > test/org/waveprotocol/box/server/waveserver/WaveMapTest.java e161490 > test/org/waveprotocol/box/server/waveserver/WaveServerTest.java 1da4f7b > test/org/waveprotocol/wave/federation/xmpp/MockDisco.java 6a0193e > test/org/waveprotocol/wave/federation/xmpp/RoundTripTest.java e7879c0 > test/org/waveprotocol/wave/federation/xmpp/XmppDiscoTest.java 9be9588 > > test/org/waveprotocol/wave/federation/xmpp/XmppFederationHostForDomainTest.java > 6994484 > test/org/waveprotocol/wave/federation/xmpp/XmppFederationRemoteTest.java > 3c19283 > > Diff: https://reviews.apache.org/r/32893/diff/ > > > Testing > ------- > > Checked that tests pass. > Checked that server runs with various deltas store/search types. > Checked data migration tool still works. > > > Thanks, > > Yuri Zelikov > >
