Hi, Istvan, do you have any findings? Does this break Phoenix on branch-2.6?
Thanks. 张铎(Duo Zhang) <palomino...@gmail.com> 于2024年6月14日周五 09:32写道： > > The PR for branch-2 > > https://github.com/apache/hbase/pull/5985 > > The PR for branch-2.6 > > https://github.com/apache/hbase/pull/5990 > > At least the UTs are all fine. > > The current PR does not do much incompatible change, the only extra > check is we require filters to return ExtendedCell instead of Cell. > > Thanks. > > 张铎(Duo Zhang) <palomino...@gmail.com> 于2024年6月13日周四 14:33写道： > > > > OK, let me open PRs for branch-2 and branch-2.6 too. > > > > Istvan Toth <st...@cloudera.com.invalid> 于2024年6月13日周四 14:02写道： > > > > > > I am also concerned about this change on branch-2. > > > Some of our code does implement getSequenceId() , so doing this on 2.5 > > > and > > > 2.6 would definitely break the existing releases, and require some > > > changes > > > and new compatibility modules on the Phoenix side. > > > > > > Phoenix usually uses the KeyValue type internally, which already extends > > > ExtendedCell, so it's probably not a huge problem, but we'd like to check > > > before this is merged to branch-2. > > > I'd like to ask for a heads-up when we have a PR for branch-2 so that we > > > can check what breaks. > > > (We currently only support branch-2.6, so ideally we'd have a branch-2.6 > > > PR > > > to check, but we can probably get branch-2 to work to check the change > > > without a lot of work) > > > > > > We also have a POC branch for HBase 3, so we can also check Phoenix with a > > > branch-3 version of this change. > > > > > > The worst case scenario for Phoenix would be having to open a different > > > branch for hbase 3.x support, we really want to avoid that. > > > As long as the change can be papered over with a compatibility module, > > > we're mostly fine. > > > > > > Istvan > > > > > > > > > > > > On Thu, Jun 13, 2024 at 5:45 AM 张铎(Duo Zhang) <palomino...@gmail.com> > > > wrote: > > > > > > > In fact, I do not think CPs can really return their own Cell > > > > implementation which is not an ExtendedCell, we will call > > > > PrivateCellUtil.setSequenceId and setTimestamp at server side, if the > > > > Cell is not an ExtendedCell we will throw IOException... > > > > > > > > I guess the only place where it is safe to return a customized Cell is > > > > in filter implementation, in getNextCellHint, where we will only use > > > > it to seek the scanners without storing it anywhere... > > > > > > > > Reid Chan <reidchan0...@gmail.com> 于2024年6月13日周四 10:56写道： > > > > > > > > > > It may affect Phoenix, as it is CPs related, at least for branch-2. > > > > > > > > > > Let's wait and see if there are any comments from them > > > > > > > > > > > > > > > --- > > > > > > > > > > Best regards, > > > > > R.C > > > > > > > > > > > > > > > > > > > > On Tue, Jun 11, 2024 at 4:19 PM Duo Zhang <zhang...@apache.org> wrote: > > > > > > > > > > > We have several deprecated methods in Cell interface, like > > > > > > getSequenceId and tag related ones, which are marked as deprecated > > > > > > since 2.0.0 and should be removed in 3.0.0. Most of them are marked > > > > > > as > > > > > > deprecated in HBASE-19112. > > > > > > > > > > > > After investigating, I found that it is not an easy work... > > > > > > > > > > > > We have 3 levels for the Cell interface > > > > > > > > > > > > Cell -> RawCell -> ExtendedCell > > > > > > > > > > > > Where Cell is the Public API for client side usage, RawCell is for > > > > > > CPs > > > > > > where we expose the tag related APIs, and ExtendedCell is for server > > > > > > side usage, where we expose all the internal stuff, like sequence > > > > > > id. > > > > > > > > > > > > In HBASE-19550, we introduced a CellWrapper as we think maybe CPs > > > > > > will > > > > > > return a Cell which is not an ExtendedCell at server side, we need > > > > > > to > > > > > > wrap it so at server side we always get an ExtendedCell. So if we > > > > > > remove the tag and sequence id related methods, CellWrapper will be > > > > > > broken. > > > > > > > > > > > > For me, I do not think we need the CellWrapper. In general, all > > > > > > actual > > > > > > Cell implementation classes in our code base implement the > > > > > > ExtendedCell interface, i.e, we can simply consider all Cells are > > > > > > actually ExtendedCells. The only reason we introduce the Cell > > > > > > interface, is to hide internal stuff from client public API, does > > > > > > not > > > > > > mean we want users to implement their own Cell. > > > > > > > > > > > > So the plan is to change server side code use ExtendedCell as much > > > > > > as > > > > > > possible in HBASE-28644, a draft PR is already there for review[1]. > > > > > > This will be landed to both 3.x and at least branch-2(I'm not sure > > > > > > if > > > > > > it worth to also land on branch-2.6 and branch-2.5). > > > > > > And starting from 3.0.0, we should make clear that all Cell > > > > > > instances > > > > > > should be created via the CellBuilder related APIs, other Cell > > > > > > implementations are not allowed. So on 3.0.0, we should treat Cell > > > > > > as > > > > > > ExtendedCell everywhere in our code base, and also remove the > > > > > > CellWrapper class, then we could finally remove the deprecated > > > > > > methods > > > > > > in the Cell interface. > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > Thanks. > > > > > > > > > > > > 1. https://github.com/apache/hbase/pull/5976 > > > > > > > > > > > > > > > > > > > -- > > > *István Tóth* | Sr. Staff Software Engineer > > > *Email*: st...@cloudera.com > > > cloudera.com <https://www.cloudera.com> > > > [image: Cloudera] <https://www.cloudera.com/> > > > [image: Cloudera on Twitter] <https://twitter.com/cloudera> [image: > > > Cloudera on Facebook] <https://www.facebook.com/cloudera> [image: Cloudera > > > on LinkedIn] <https://www.linkedin.com/company/cloudera> > > > ------------------------------ > > > ------------------------------
