> On Sept. 17, 2024, 5:15 a.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. > > Radhika Kundam wrote: > 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.
ok. I was trying to keep the perf traces for both methods, batch and single, separate. > On Sept. 17, 2024, 5:15 a.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. > > Radhika Kundam wrote: > 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. How about declaring 'resource' outside the try block? - Madhan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/75208/#review226916 ----------------------------------------------------------- On Sept. 16, 2024, 7: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, 7: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 > >
