Since PHOENIX-7331 has been resolved, I will also merge the PR for HBASE-28644,
Thanks all for helping moving forward here! Istvan Toth <st...@cloudera.com.invalid> 于2024年6月17日周一 13:37写道： > > Hi Duo, > > Yes, it does: > > [ERROR] Failed to execute goal > org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile > (default-compile) on project phoenix-core-client: Compilation failure: > Compilation failure: > [ERROR] > /home/stoty/workspaces/apache-phoenix/phoenix/phoenix-core-client/src/main/java/org/apache/phoenix/util/PhoenixKeyValueUtil.java:[262,93] > incompatible types: java.util.List<org.apache.hadoop.hbase.Cell> cannot be > converted to java.util.List<org.apache.hadoop.hbase.ExtendedCell> > [ERROR] > /home/stoty/workspaces/apache-phoenix/phoenix/phoenix-core-client/src/main/java/org/apache/phoenix/hbase/index/util/IndexManagementUtil.java:[248,69] > incompatible types: java.util.List<org.apache.hadoop.hbase.Cell> cannot be > converted to java.util.List<org.apache.hadoop.hbase.ExtendedCell> > > Now, we haven't actually released (TBH we haven't even committed it) Hbase > 2.6 support yet, so technically this doesn't break any released Phoenix > code. > > I played around with it a bit, and it looks like those two errors can be > fixed in a backwards-compatible manner with some work. > > Opened https://issues.apache.org/jira/browse/PHOENIX-7331 to track that > work. > > > On Mon, Jun 17, 2024 at 3:12 AM 张铎(Duo Zhang) <palomino...@gmail.com> wrote: > > > 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> > > > > > ------------------------------ > > > > > ------------------------------ > > > > > -- > *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> > ------------------------------ > ------------------------------
