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> > ------------------------------ > ------------------------------ >
