> On March 15, 2014, 2:39 p.m., Jihoon Son wrote:
> > Hi, Dae Myung. Many thanks for your report and patch.
> >
> > I have a question about your report.
> > TajoMaster and CatalogServer extend CompositeService and AbstractService,
> > respectively.
> > When super.init() is called in TajoMaster.init(), CatalogServer.init() is
> > called via CompositeService.serviceInit().
> > As you can see, the configuration looks to be assigned in
> > CatalogServer.init().
> > So, I think that the configuration is assigned successfully.
> > (When I check the log of TajoMaster, I found the following line.
> > 2014-03-10 09:09:35,297 INFO catalog.CatalogServer
> > (CatalogServer.java:init(111)) - Catalog Store Class:
> > org.apache.tajo.catalog.store.DerbyStore)
> > If I missed any other problems during the initialization of CatalogServer,
> > please correct me.
> >
> > Thanks,
> > Jihoon
>
> Dae Myung Kang wrote:
> @Jihoon Yes. you're almost right. I just found some irregular case. and
> It teased me.
>
> finally. CatalogServer.init() is called. and that time catalogServer can
> get tajoConf.
>
> but we should see that parts. it is "public void init(Configuration
> _conf)" in TajoMaster.java
>
> ```java
> catalogServer = new CatalogServer(initBuiltinFunctions(),
> systemConf);
> addIfService(catalogServer);
> catalog = new LocalCatalogWrapper(catalogServer);
> ```
>
> before calling catalogServer.init() function(in CompositeService),
> LocalCatalogWrapper already uses tajoConf like below in its Constructor.
>
> ```java
> public LocalCatalogWrapper(final CatalogServer server) {
> super(server.getConf(), null);
> this.catalog = server;
> this.stub = server.getHandler();
> }
>
> ```
>
> so it can cause null pointer exception if someone call tajoconf from
> LocalCatalogWrapper.
>
> Thank you.
>
>
> Jihoon Son wrote:
> Thanks for the further explanation. I missed that part.
> Your patch looks to be enough to solve the aforementioned problem.
> But, TajoConf may be assigned repeatedly (in the constructor and the
> init() function), and it will be definitely an unnecessary operation.
> Would you improve the patch to handle this repeated assignment?
>
> Dae Myung Kang wrote:
> @Jihoon
>
> Yes. but catalog(LocalCatalogWrapper) is not (AbstractService or
> CompositeServer) class.
> so even though, CatalogServer will get TajoConf from init call(), catalog
> can't get it.
>
> and it is connectted to RpcConnectionPool(Singleton)
>
> public synchronized static RpcConnectionPool getPool(TajoConf conf) {
> if(instance == null) {
> instance = new RpcConnectionPool(conf,
> RpcChannelFactory.getSharedClientChannelFactory());
> }
> return instance;
> }
>
> so, it is called first. RpcConnectionPool can't get TajoConf anymore.
> and someone can do a mistake when using TajoConf.
>
> so I think it should be fixed.
>
>
> Jihoon Son wrote:
> Yes, you are right. This problem should be fixed.
> Maybe, I made you confused. Sorry.
>
> My above comment means that CatalogServer.TajoConf will be repeatedly
> assigned in the constructor of CatalogServer and CatalogServer.init().
> This is very trivial, but I think that we don't have to make the code
> more complex.
>
> So, I believe that there is a better solution.
> How about change the constructor of LocalCatalogWrapper as follows?
>
> LocalCatalogWrapper(final CatalogServer server) =>
> LocalCatalogWrapper(final TajoConf conf, final CatalogServer server)
>
> Or, do you have any good idea?
>
> Dae Myung Kang wrote:
> Yes. I also thought two things as solutions.
>
> 1] changing LocalCatalogWrapper's constructor (jihoon's suggestion)
> 2] changing CatalogServer's constructor(my patch)
>
> I think 1] is also good.
> but I might think if someone uses catalogServer like LocalCatalogWrapper
> He will make a mistake like me.(Because I still don't know well
> CatalogServer's usage as much as you.
>
> So If there is no possibility to use CatalogServer like
> LocalCatalogWrapper.
>
> I will choose your solution.
> Could I change it? :) Thank you for your review.
>
> Jihoon Son wrote:
> Thanks for the reply, Dae Myung.
>
> It is hard to convince that there isn't any other applications such as
> LocalCatalogWrapper.
> If some other applications using CatalogServer (, AbstractService or
> CompositeService) are required to be developed, users may make a mistake as
> you said.
> However, in my opinion, it seems a problem of the lack of documents and
> inline comments.
>
> So, I suggest to change the constructor of LocalCatalogWrapper, and leave
> some inline comments at CatalogServer for its usage.
> How do you think about it?
@Jihoon
Yes. I agree with you.
I will patch again :)
Thank you.
- Dae Myung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19221/#review37317
-----------------------------------------------------------
On March 14, 2014, 1:27 p.m., Dae Myung Kang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19221/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 1:27 p.m.)
>
>
> Review request for Tajo.
>
>
> Bugs: https://issues.apache.org/jira/browse/TAJO-687
>
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/TAJO-687
>
>
> Repository: tajo
>
>
> Description
> -------
>
>
> TajoMaster currently doesn't pass tajoConf to create catalogServer. so it
> create RpcConnectionPool with null tajoConf.
>
> so it can cause NullPointerException if you use tajo conf in NettyClientBase
>
>
> Diffs
> -----
>
>
> tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/CatalogServer.java
> 621b475
>
> tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/TajoMaster.java
> c42b0f3
>
> Diff: https://reviews.apache.org/r/19221/diff/
>
>
> Testing
> -------
>
> pass "mvn clean install"
>
>
> Thanks,
>
> Dae Myung Kang
>
>
