Hey Soumyakanti, Thanks for starting this discussion.
I like the idea of reducing boilerplate code and one way although not the only one is using AOP. AOP libraries rely on code injection and there are various pros/cons [1] when using such tools. If AspectJ is the best option for this use-case I have no objections using it. The PerfLogger component is called quite often during the lifecycle of a query so when introducing changes we should be mindful of the performance overhead that they may introduce. Before merging the change we should at least have some basic benchmark to ensure that the instrumentation performed by AspectJ (or another code injection tool) is not too expensive. If there are no significant perf differences and the code becomes more readable after this change then it definitely makes sense to move this forward. Best, Stamatis [1] https://dzone.com/articles/practical-introduction-code On Mon, Feb 5, 2024 at 1:19 AM Soumyakanti Das <soumyakanti....@cloudera.com.invalid> wrote: > > Hi all, > > Do you guys think it's a good idea to implement annotations for PerfLogger? > Currently, we have to surround the code with a PerfLogBegin and a > PerfLogEnd to log execution time. There are many methods where the first > and last line are these. Instead, we could use AOP to create an annotation > and annotate a method. > > I have done a small POC with AspectJ: > https://github.com/soumyakanti3578/hive/tree/annotated-perf-logger > <https://github.com/soumyakanti3578/hive/commit/ff7d94cfe77e00d48a9edb389816efd5c4cd6349> > I had to just add 2 dependencies to hive-exec, create an annotation > (@LogPerf), and an Aspect (PerfLoggerAspect). I tested it on my local > machine and it works well. > > PerfLogBegin takes 2 arguments - callerName, and method. And PerfLogEnd > takes an additional argument - additionalInfo. These three arguments can be > passed through the annotation. > > I think this will help clean up our source code a little bit. Please let me > know what you think about this. > > Thanks, > Soumyakanti Das
