> 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?
@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.
- 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
>
>
