My opinion depends on what we think the timeframe for releasing 5.2 would be.
If it's soon, then we can focus on that and let 5.1 be more for bug fixes and security updates after the 5.1.4 release. If it's not soon (and I know there are still large ongoing feature branches for JSON support, CDC, and the new Metadata API) then the ongoing maintenance burden of having two very different release branches probably makes the backport of the client/server change worthwhile. Geoffrey On Wed, Dec 13, 2023 at 9:03 PM Istvan Toth <st...@cloudera.com.invalid> wrote: > 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> > ------------------------------ > ------------------------------ >
