> On 九月 20, 2017, 2:56 p.m., Colm O hEigeartaigh wrote: > > I would change the first line to also include the serviceType. Something > > like "could not find the service-def for the service-type '" + serviceType > > + "'" > > Secondly, I don't understand the need for the second change, the > > ServiceType is empty here so why do we need to log it?
1. Your following advice is very good, I will modify it according to your comments. Something like "could not find the service-def for the service-type '" + serviceType + "'" 2. Secondly, I don't understand the need for the second change, the ServiceType is empty here so why do we need to log it? I have the same question as you. There are many ways to modify it. But a reasonable modification will change the call logic of the function. This could introduce new bugs. So my opinion is to keep the achieve logic of the original program to avoid introducing new issues. Do you agree with me? - Qiang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62430/#review185796 ----------------------------------------------------------- On 九月 20, 2017, 8:10 a.m., pengjianhua wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62430/ > ----------------------------------------------------------- > > (Updated 九月 20, 2017, 8:10 a.m.) > > > Review request for ranger, Alok Lal, Ankita Sinha, Don Bosco Durai, Colm O > hEigeartaigh, Gautam Borad, Madhan Neethiraj, Ramesh Mani, Selvamohan > Neethiraj, Velmurugan Periasamy, and Qiang Zhang. > > > Bugs: RANGER-1794 > https://issues.apache.org/jira/browse/RANGER-1794 > > > Repository: ranger > > > Description > ------- > > if(! StringUtils.isEmpty(serviceType)) { > RangerServiceDef serviceDef = svcStore == null ? null : > svcStore.getServiceDefByName(serviceType); > if(serviceDef != null) { > Class<RangerBaseService> cls = > getClassForServiceType(serviceDef); > if(cls != null) { > ret = cls.newInstance(); > ret.init(serviceDef, service); > if(ret instanceof RangerServiceTag) { > ((RangerServiceTag)ret).setTagStore(tagStore); > } > } else { > LOG.warn("ServiceMgr.getRangerServiceByService(" + > service + "): could not find service class '" + serviceDef.getImplClass() + > "'"); > } > } else { > LOG.warn("ServiceMgr.getRangerServiceByService(" + service + > "): could not find the service-type '" + serviceType + "'"); > } > } else { > LOG.warn("ServiceMgr.getRangerServiceByService(" + service + "): could > not find the service-type"); > } > The above code should be modified as following: > if(! StringUtils.isEmpty(serviceType)) { > RangerServiceDef serviceDef = svcStore == null ? null : > svcStore.getServiceDefByName(serviceType); > if(serviceDef != null) { > Class<RangerBaseService> cls = > getClassForServiceType(serviceDef); > if(cls != null) { > ret = cls.newInstance(); > ret.init(serviceDef, service); > if(ret instanceof RangerServiceTag) { > ((RangerServiceTag)ret).setTagStore(tagStore); > } > } else { > LOG.warn("ServiceMgr.getRangerServiceByService(" + > service + "): could not find service class '" + serviceDef.getImplClass() + > "'"); > } > } else { > LOG.warn("ServiceMgr.getRangerServiceByService(" + service + > "): could not find the service-def"); > } > } else { > LOG.warn("ServiceMgr.getRangerServiceByService(" + service + "): could > not find the service-type '" + serviceType + "'"); > } > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/biz/ServiceMgr.java 722a5662 > > > Diff: https://reviews.apache.org/r/62430/diff/1/ > > > Testing > ------- > > > Thanks, > > pengjianhua > >
