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