You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by pengjianhua <pe...@zte.com.cn> on 2017/09/20 08:10:14 UTC
Review Request 62430: The warning information is incorrect in
getRangerServiceByService of ServiceMgr class
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62430/
-----------------------------------------------------------
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
Re: Review Request 62430: The warning information is incorrect in
getRangerServiceByService of ServiceMgr class
Posted by Qiang Zhang <zh...@zte.com.cn>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62430/#review185778
-----------------------------------------------------------
Ship it!
Ship It!
- Qiang Zhang
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
>
>
Re: Review Request 62430: The warning information is incorrect in
getRangerServiceByService of ServiceMgr class
Posted by Colm O hEigeartaigh <co...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62430/#review186124
-----------------------------------------------------------
Ship it!
Ship It!
- Colm O hEigeartaigh
On Sept. 25, 2017, 7:22 a.m., pengjianhua wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62430/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2017, 7:22 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> pengjianhua
>
>
Re: Review Request 62430: The warning information is incorrect in
getRangerServiceByService of ServiceMgr class
Posted by pengjianhua <pe...@zte.com.cn>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62430/
-----------------------------------------------------------
(Updated 九月 25, 2017, 7:22 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 (updated)
-----
security-admin/src/main/java/org/apache/ranger/biz/ServiceMgr.java 722a5662
Diff: https://reviews.apache.org/r/62430/diff/2/
Changes: https://reviews.apache.org/r/62430/diff/1-2/
Testing
-------
Thanks,
pengjianhua
Re: Review Request 62430: The warning information is incorrect in
getRangerServiceByService of ServiceMgr class
Posted by pengjianhua <pe...@zte.com.cn>.
> 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?
>
> Qiang Zhang wrote:
> 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?
Hi Colm, I had modified the issue and rebuilt the patch according your review. Thanks.
- pengjianhua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62430/#review185796
-----------------------------------------------------------
On 九月 25, 2017, 7:22 a.m., pengjianhua wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62430/
> -----------------------------------------------------------
>
> (Updated 九月 25, 2017, 7:22 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> pengjianhua
>
>
Re: Review Request 62430: The warning information is incorrect in
getRangerServiceByService of ServiceMgr class
Posted by Qiang Zhang <zh...@zte.com.cn>.
> 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
>
>
Re: Review Request 62430: The warning information is incorrect in
getRangerServiceByService of ServiceMgr class
Posted by Colm O hEigeartaigh <co...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62430/#review185796
-----------------------------------------------------------
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?
- Colm O hEigeartaigh
On Sept. 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 Sept. 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
>
>