> On Sept. 16, 2024, 10:15 p.m., Madhan Neethiraj wrote: > > security-admin/src/main/java/org/apache/ranger/rest/GdsREST.java > > Line 1040 (original), 1044 (patched) > > <https://reviews.apache.org/r/75208/diff/1/?file=2293268#file2293268line1044> > > > > I suggest to retain existing implementation in addSharedResource() and > > removeSharedResource() i.e. don't forward to the new method with a > > singleton collection.
To avoid redundancy, I tried to keep the logic for adding shared resources in one place, either in addSharedResource() or addSharedResources(). If I keep the logic in addSharedResource() as suggested, it will print logs for each individual resource in debug mode, which could result in an excessive amount of logs. By moving the logic to a common method that can be called by both addSharedResource() and addSharedResources(), we avoid this issue, but it unnecessarily increases complexity, where the current code in patch handling the same. Please let me know if the increased number of debug logs per resource is acceptable, and I’ll proceed with the suggested changes accordingly. > On Sept. 16, 2024, 10:15 p.m., Madhan Neethiraj wrote: > > security-admin/src/main/java/org/apache/ranger/rest/GdsREST.java > > Line 1044 (original), 1068 (patched) > > <https://reviews.apache.org/r/75208/diff/1/?file=2293268#file2293268line1068> > > > > It may not be useful to print perf log for every iteration. Consider > > replacing with the following: > > > > > > try { > > if(RangerPerfTracer.isPerfTraceEnabled(PERF_LOG)) { > > perf = RangerPerfTracer.getPerfTracer(PERF_LOG, > > "GdsREST.addSharedResource(" + resource + ")"); > > } > > > > for (RangerSharedResource resource : resources) { > > ret.add(gdsStore.addSharedResource(resource)); > > } > > } catch(WebApplicationException excp) { > > throw excp; > > } catch(Throwable excp) { > > LOG.error("addSharedResources({}) failed", resources, excp); > > > > throw restErrorUtil.createRESTException(excp.getMessage()); > > } finally { > > RangerPerfTracer.log(perf); > > } > > > > Similar update for removeSharedResources() as well. Sure, I'll move the perf log out of the for loop as suggested. However, enclosing the entire loop in a try-catch block makes it harder to identify which specific resource caused an exception during the addition. The catch block would only provide details about all resources processed, not the specific one that failed. Please let me know your thoughts on this. - Radhika ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/75208/#review226916 ----------------------------------------------------------- On Sept. 16, 2024, 12:14 p.m., Radhika Kundam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/75208/ > ----------------------------------------------------------- > > (Updated Sept. 16, 2024, 12:14 p.m.) > > > Review request for ranger, Madhan Neethiraj and Ramesh Mani. > > > Bugs: RANGER-4934 > https://issues.apache.org/jira/browse/RANGER-4934 > > > Repository: ranger > > > Description > ------- > > Ranger API to add and delete assets to the DataShare in bulk. Current API > accepts only one resource and has to support adding / deleting multiple > resources. > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/rest/GdsREST.java b1a00533e > > > Diff: https://reviews.apache.org/r/75208/diff/1/ > > > Testing > ------- > > Tested manually. > > > File Attachments > ---------------- > > API details > > https://reviews.apache.org/media/uploaded/files/2024/09/16/a4540abb-1c2d-471e-bbd3-eb74a8756159__Multiple_resources_add_delete_API_details.json > > > Thanks, > > Radhika Kundam > >
