Thank you both! Sorry for the long silence, I just learned today that async-profile 3 has been released which has at least one breaking change https://github.com/async-profiler/async-profiler/blob/master/CHANGELOG.md
I'll take a look at whether it is possible to just jump to that instead. I'll pick this up later and will update here. On Thu, Jan 18, 2024 at 6:28 PM Andrew Purtell <andrew.purt...@gmail.com> wrote: > > 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> > >>>> > >>
