As long as 5.1.4 can be released right away (without this backport), we should be fine IMO.
I also agree that if 5.2.0 is still going to take some time, we can backport this to 5.1 branch after 5.1.4 is released. On Thu, Dec 14, 2023 at 6:40 AM Geoffrey Jacoby <gjac...@apache.org> wrote: > 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> > > ------------------------------ > > ------------------------------ > > >
