incubator-wave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ali Lown" <...@lown.me.uk>
Subject Re: Review Request 32893: Replaces custom config framework with typesafe config
Date Tue, 07 Apr 2015 12:22:50 GMT

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


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