Thanks. Sure. In case we decide to backport, I'd still want to wait a few weeks to shake out any problems before backporting.
Istvan On Thu, Dec 14, 2023 at 3:09 AM Viraj Jasani <vjas...@apache.org> wrote: > Sure, 5.2.0 sounds good. > > Reg the backport to 5.1 branch, I am in bit of a dilemma. Let's wait some > time for more opinions? > > > Thanks, > Viraj > > > On Wed, Dec 13, 2023 at 2:31 AM Istvan Toth <st...@cloudera.com.invalid> > wrote: > > > Thanks for responding, Viraj > > > > On compatibility: > > > > I am confident that this patch does not affect compatibility at all. > > The wire protocol remains the same, we are using the same protobuf > > definitions, and we use them identically. > > The classes which are referred from the HBase configuration or Hbase > > metadata (coprocessors, SplitPolicy, etc) > > have retained their names, and their behaviour. > > > > The only way this change can cause problems is: > > > > - We have made a mistake during refactoring, and changed behaviour. This > > would be a bug that can be fixed. > > - An application uses a refactored internal class directly. This is > > unlikely, and even if it happens, this can happen with any patch. > > > > About 99 percent of the changes is one of these two things: > > - Move the string constants out of the coprocessors into helper classes > in > > the client module > > - Split the static utility classes that contain methods used both from > the > > server and client side into two classes. > > > > The remaining one percent was somewhat more complex, where the client and > > server side code was more intertwined, and > > required actual thinking on how to solve. > > > > If you ignore the class (and sometime) method name changes, then both the > > client and server should execute exactly the same code. > > Aron has made some minor optimizations in a handful of cases, if those > turn > > out to be incorrect, then we can fix or revert them. > > > > I would compare this change to the one where we added the compatibility > > modules. > > We have changed the maven project structure heavily, and touched a lot of > > files, and added interfaces and abstract classes to handle this, but > there > > was zero change in the behaviour of the code. > > > > Regarding the 5.2/6.0 version question: > > > > This is more of an aesthetic question. The last major version change was > > for HBase 2.0. > > I am hopeful that we will be able to find a way to support HBase 3.x > > without branching the code base. > > I don't really see the need for a new major release. We have dropped the > > ball when we did not release more minor versions during the last 2+ > years. > > We should be talking about releasing 5.4 or 5.5 by now. > > The new release doesn't break compatibility, so I see no technical reason > > to go to 6.0. > > Of course, your point of having a lot of changes is valid, if the > community > > agrees then going with 6.0 is also fine. > > > > Regarding the artifacts: > > > > We have found a last-minute solution to minimize the visible changes both > > for the consumers of the maven artifacts and the shaded JARs. > > By retaining phoenix-core, and making it depend on both of the new > modules, > > downstream applications should not need to make any changes in their > > dependencies. > > (Of course it is recommended to depend on phoenix-core-client instead for > > JDBC users) > > The client and server JARs also contain exactly the same code, as they > > depend on phoenix-core, and phoenix-core-server and both include both the > > client and server side code, exactly as they did before, with exactly the > > same relocations. > > ( phoenix-core-server depends on phoenix-core-client, so depending on it > is > > effectively the same as depending on phoenix-core) > > My original proposal included making changes in phoenix-server, but the > > committed change does not include that. > > The shaded JARs with different content will be new jars, with new names > > (see my first email) > > > > > > Regarding 5.1: > > > > I hope that the above can sway your opinion. > > > > If you have any more questions and concerns then I'm more than happy to > > discuss them. > > > > Best Regards > > Istvan > > > > On Wed, Dec 13, 2023 at 6:19 AM Viraj Jasani <vjas...@apache.org> wrote: > > > > > One more question: do generated jars (phoenix-client and > phoenix-server) > > > follow the same naming conventions after this change? Perhaps this is > > > already answered somewhere, I hope I can take a thorough look at the > Jira > > > discussions and the change soon :) > > > > > > > > > Thanks, > > > Viraj > > > > > > > > > On Tue, Dec 12, 2023 at 8:59 PM Viraj Jasani <vjas...@apache.org> > wrote: > > > > > > > Thank you for starting this thread Istvan! > > > > > > > > This is really nice change and I can see how porting to 5.1 could be > > > > beneficial as well as disastrous for a maintenance release. One > > question: > > > > how confident are we from backward compatibility viewpoint? > > specifically > > > > 5.1 client against current master branch based server. > > > > > > > > IMHO, we should keep this for major/minor release only. It would also > > > > enforce us to create 2 PRs going forward (one for master/5.2 and > > another > > > > for 5.1) but this is still better in case something gets broken in > the > > > > future. Fixing it on maintenance release would also be nightmare. > > > > > > > > This is really good improvement. I also wonder what should be our > path > > > > forward beyond 5.1. Shall we still stick to 5.2.0 or move to 6.0.0? > > > > > > > > master branch is becoming extremely heavy day by day with many > features > > > > still being lined up for merging very soon. Json support is almost > > ready > > > I > > > > believe. We have data integrity issues and many behavioural changes > > that > > > > are non-compatible (but necessary to implement heavy feature like > > > ViewTTL) > > > > in the queue. Many committed features like Partial Index, Uncovered > > > Index, > > > > MasterRegistry compatible JDBC connection etc are ready for rollout > as > > > soon > > > > as we cut release branch from master. It does seem quite a massive > list > > > for > > > > a minor release, but this is just my opinion. I am not against 5.2.0 > > > > release, would be open for more opinions. > > > > > > > > Thanks, > > > > Viraj > > > > > > > > > > > > On Mon, Dec 11, 2023 at 11:31 PM Istvan Toth <st...@apache.org> > wrote: > > > > > > > >> Hi! > > > >> > > > >> First of all, a heads up that I have merged the Client-Server code > > > >> separation (PHOENIX-6053) patch a few minutes ago. > > > >> > > > >> A huge thanks to Aron for turning my two years old POC patch into a > > > >> merge-ready patch, and handling all the change requests and numerous > > > >> rebases. > > > >> > > > >> The patch has been discussed on the ticket > > > >> <https://issues.apache.org/jira/browse/PHOENIX-6053> > > > >> and in the 2021 > > > >> <https://lists.apache.org/thread/hs4klbc04n4gh62z17pznc0rkspjg6jx> > > and > > > >> the > > > >> recent > > > >> < > > > >> > > > > > > https://lists.apache.org/list?dev@phoenix.apache.org:2023-10:PHOENIX-6053 > > > >> > > > > >> email threads. > > > >> > > > >> > > > >> *A very quick recap:* > > > >> *phoenix-core* has been split into two modules* phoenix-core-client* > > and > > > >> *phoenix-core-server*. > > > >> *Phoenix-core-server* depends on* phoenix-core-client*, as we more > or > > > less > > > >> need the full client code on the server side. > > > >> phoenix-core includes all the code needed the JDBC driver, but does > > not > > > >> include anything that is used exclusively on the server side > > > >> (coprocessors, > > > >> split policies, etc) > > > >> > > > >> *phoenix-core* is still around, it retains all the tests (as the > > > majority > > > >> of them ), and it also acts as a compatibility dependency, which > > > >> transitively depends on both client and server. > > > >> > > > >> > > > >> *Backporting to 5.1:* > > > >> We need to make a decision on whether to backport this change into > 5.1 > > > or > > > >> not. > > > >> > > > >> Against: > > > >> > > > >> - It is a huge patch by file count > > > >> - maven module structure changes > > > >> > > > >> For: > > > >> > > > >> - Backports to 5.1 will be a *nightmare* if we don't backport > this > > > >> change > > > >> - The actual code changes are minimal, and the behavioural > changes > > > >> should be non-existent. > > > >> - Dependency compatibility should be handled by the phoenix-core > > > >> package > > > >> - Users get the advantages without having to wait for 5.2. > > > >> > > > >> We have three options: > > > >> > > > >> - Backport to 5.1.4 > > > >> - Backport post 5.1.4 > > > >> - Do not backport > > > >> > > > >> > > > >> *I really need to hear your take on this.* > > > >> > > > >> *Next steps:* > > > >> This change in itself does little apart from cleaning up the code, > but > > > it > > > >> enables a number of important new features (these were originally > > > planned > > > >> to be included in PHOENIX-6053, but I decided to split them to > > separate > > > >> tickets to keep the scope manageable): > > > >> > > > >> > > > >> - PHOENIX-7137 < > https://issues.apache.org/jira/browse/PHOENIX-7137 > > > > > > >> Create *phoenix-client-lite* shaded JAR without server-side > > > >> dependencies > > > >> - This adds a new shaded client variant, *phoenix-client-lite > > > >> *(names > > > >> are up for discussion), which omits the server-side code and > its > > > >> dependencies. It is ~20 MB smaller, and pollutes the classpath > > > less. > > > >> - PHOENIX-7139 < > https://issues.apache.org/jira/browse/PHOENIX-7139 > > > > > > >> Create *phoenix-mapreduce-byo-shaded-hbase* artifact for use by > > > >> connectors > > > >> - This allows using the hbase-shaded-client and phoenix on the > > > same > > > >> classpath. Up until now, you had to do that by using the > > > >> phoenix-core > > > >> dependency from Maven, or by using the HBase libraries shaded > > into > > > >> phoenix-client. > > > >> The current phoenix-client will fail hard with any other Hbase > > > >> libraries on the classpath due to relocation conflicts. > > > >> - Allows updating the hbase client code without rebuilding > > Phoenix > > > >> - Solves the protobuf 2.5.0 woes, which make it impossible to > > use > > > >> code/libraries using unshaded protobuf 3.0 together with > > > >> Phoenix. This was, > > > >> and remains the original driver behind PHOENIX-6053. > > > >> - Allows using Hadoop extensions with Phoenix. Being able to > use > > > the > > > >> various cloud connectors like AWS S3a without shading hundreds > > > >> of megabytes > > > >> of additional code into phoenix-client was another driver for > > this > > > >> change. > > > >> - This is the same shading setup already used by the shaded > Hive > > > and > > > >> Spark connector artifacts. > > > >> - Create a phoenix-client-byo-shaded-hbase which would be the > same > > as > > > >> *phoenix-mapreduce-byo-shaded-hbase*, but omit the phoenix > > > server-side > > > >> code. > > > >> - I'm on the fence about this. > > phoenix-mapreduce-byo-shaded-hbase > > > >> should be usable for this purpose. The server-side phoenix > code > > > >> is not that > > > >> big, and *phoenix-mapreduce-byo-shaded-hbase *already omits > any > > > >> server-side dependencies which could cause conflicts. > > > >> - Will need to run to see if this is needed / useful. > > > >> > > > > > > > > > > > > > -- > > *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> ------------------------------ ------------------------------
