I was the original developer of this feature. It existed in pretty much the same form in Hive and was a straightforward port. The style of using enums to encode and restrict supported parameters came from the origin. I thought it a good idea, though, because this shells out to an external process so is inherently risky. Make a mistake and it’s probably a CVE worthy issue.
I second Bryan’s recommendation to keep compatibility. In the worst case there can be two implementation classes selectable by site configuration. > On Jan 18, 2024, at 6:03 AM, Bryan Beaudreault <bbeaudrea...@apache.org> > wrote: > > Yea, that makes sense and was the one reason I could think of as well. > > My concern with not backporting is that 3.x has been in development for > many years, and all of our users are on 2.x (or even 1.x). Duo is making > progress on 3.x now, but it's not clear when it will be released or, > further, when people will upgrade. So IMO features that land just in > master/branch-3 may not see any users for a long time. All of the active > development for users gets backported to 2.x, and unnecessarily keeping big > diffs between 3.x and 2.x just increases the backport burden (merge > conflicts). There are some good reasons for certain major feature changes > to have such a diff, but not sure it warrants it for this. > > I am guessing the changes in your PR which break v1 are related to either > removal of certain enum options or what the default values are. I suggest > that we _do_ backport this to branch-2, but in that backport we add back > whatever enum options are necessary to keep support for both v1 and v2. We > can say that v1 is dropped in 3.x. If necessary, we can add a config in > branch-2 to specify which version of async-profiler to use (like if we need > to determine which default values to use). > > Thoughts? > >> On Thu, Jan 18, 2024 at 8:42 AM Lars Francke <lars.fran...@gmail.com> wrote: >> >> Hi Bryan, >> >> good question. We can probably keep supporting v1 if we wanted to but >> I'm wondering if it's worth the effort (personally I think not). >> I do agree that it should not be backported! >> >> And I have wondered the same. If you look at the history of the issue >> I kinda proposed forwarding parameters as well but then thought that >> it would mean giving someone "access" to a shell with HBase privileges >> and we'd need to deal with sanitizing inputs and making sure people do >> nothing malicious. >> That was a task that I wasn't prepared to tackle so I opted to keep >> the current route and just update it. >> >> I hope that makes sense? >> >> Cheers, >> Lars >> >>> On Thu, Jan 18, 2024 at 2:16 PM Bryan Beaudreault >>> <bbeaudrea...@apache.org> wrote: >>> >>> Is it necessary to break support for v1? >>> >>> It would be fine to merge this to master and branch-3. But I think this >>> change falls into the "Dependency Compatibility" portion of our >>> compatibility guidelines[1]. If so, then we should not backport to >> branch-2 >>> or 2.x. >>> >>> Generally, I have previously wondered what value the Event and Output >> enums >>> provide. It requires us to keep the enums updated with async-profiler >>> options, when we could just directly send the user's input to >>> async-profiler. If they specify a bad option, maybe we can show them the >>> error message from async-profiler. This approach might allow for any >>> async-profiler version. >>> >>> [1] https://hbase.apache.org/book.html#hbase.versioning.post10 >>> >>> On Thu, Jan 18, 2024 at 3:45 AM Lars Francke <lars.fran...@gmail.com> >> wrote: >>> >>>> Hi, >>>> >>>> I just wanted to give a heads-up on an issue I created (thanks for >>>> everyone's help)[1] >>>> >>>> This would be a breaking change as it removes support for >>>> async-profiler v1 but in exchange it does add support for various v2 >>>> features and in general "unbreaks" things that were broken. >>>> >>>> I already have an approved PR[2] but wanted to give the chance to >>>> comment before I merge that sometimes in the next few days or weeks >>>> >>>> Cheers, >>>> Lars >>>> >>>> >>>> [1] <https://issues.apache.org/jira/browse/HBASE-28242> >>>> [2] <https://github.com/apache/hbase/pull/5566> >>>> >>
