Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Phoenix 5.2.0 and HBase 2.5 #21569

Merged
merged 1 commit into from
May 7, 2024
Merged

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Apr 16, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Apr 16, 2024
@wendigo
Copy link
Contributor Author

wendigo commented Apr 16, 2024

Screenshot 2024-04-16 at 15 37 22

@wendigo wendigo force-pushed the serafin/phoenix-5.2.0 branch from 718bade to ab0c5fa Compare April 16, 2024 18:08
@wendigo wendigo changed the title Update to Phoenix 5.2.0 Update to Phoenix 5.2.0 and HBase 2.5 Apr 16, 2024
@wendigo wendigo requested review from losipiuk and kokosing April 16, 2024 18:08
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % comment

pom.xml Show resolved Hide resolved
@kokosing
Copy link
Member

Is this related?
image

@lhofhansl
Copy link
Member

@kokosing I tried what that test does locally with the PR applied, also tried many more rows with parallel writers, etc. It all works here.
So I am suspecting a problem with the testing server.

@kokosing
Copy link
Member

Maybe the new version requires more resources that CI runner provides?

@wendigo
Copy link
Contributor Author

wendigo commented Apr 23, 2024

@kokosing doubt that, it stales on my local machine randomly as wel

@wendigo
Copy link
Contributor Author

wendigo commented Apr 23, 2024

Closing. I won't invest more time to this. There is no interest in the community to keep this connector alive.

@wendigo wendigo closed this Apr 23, 2024
@lhofhansl
Copy link
Member

lhofhansl commented Apr 24, 2024

There is no interest in the community to keep this connector alive.

@virajjasani , @stoty, is that so? You guys don't care?

@virajjasani
Copy link

@virajjasani
Copy link

We can definitely help with test failures too, just need a bit more context. @lhofhansl @wendigo how are you running all Phoenix tests locally? Let me also run and confirm how things look.

@virajjasani
Copy link

I am going to run TestPhoenix on IDE and will get back here.

@virajjasani
Copy link

@lhofhansl do we run PhoenixQueryRunner for running tests on IDE?
It doesn't seem to be able to bring up miniHBase cluster.

java.lang.RuntimeException: Master not initialized after 200000ms
	at org.apache.hadoop.hbase.util.JVMClusterUtil.waitForEvent(JVMClusterUtil.java:229)
	at org.apache.hadoop.hbase.util.JVMClusterUtil.startup(JVMClusterUtil.java:197)
	at org.apache.hadoop.hbase.LocalHBaseCluster.startup(LocalHBaseCluster.java:413)
	at org.apache.hadoop.hbase.MiniHBaseCluster.init(MiniHBaseCluster.java:259)
	at org.apache.hadoop.hbase.MiniHBaseCluster.<init>(MiniHBaseCluster.java:116)
	at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniHBaseCluster(HBaseTestingUtility.java:1169)
	at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniHBaseCluster(HBaseTestingUtility.java:1213)
	at io.trino.plugin.phoenix5.TestingPhoenixServer.<init>(TestingPhoenixServer.java:81)
	at io.trino.testing.SharedResource.getInstanceLease(SharedResource.java:46)
	at io.trino.plugin.phoenix5.TestingPhoenixServer.getInstance(TestingPhoenixServer.java:44)
	at io.trino.plugin.phoenix5.PhoenixQueryRunner.main(PhoenixQueryRunner.java:137)

I can debug this further, but not sure about the location of the logs.

For now, i am just trying to get the test running (without upgrading Phoenix to 5.2.0) on IDE.

@stoty
Copy link

stoty commented Apr 24, 2024

Minicluster startup failures are usually caused by using HBase artifacts not built for Hadoop 3.
HBase 2.4.x needs to be rebuilt locally with Hadoop 3, for HBase 2.5 there are public artifacts available compiled with Hadoop3.

@lhofhansl
Copy link
Member

Oh, it might all work for me because I happen to have an HBase version built against Hadoop 3 in my local Maven repo.
Anyway, let's not discuss this here. @virajjasani I'll find you today and we can talk about this.

@wendigo Have some patience with us, I know this is frustrating.

@wendigo wendigo reopened this Apr 26, 2024
@wendigo
Copy link
Contributor Author

wendigo commented Apr 26, 2024

On the product test environment I can see an exception that I think is related to a test being stuck @stoty @lhofhansl:

2024-04-26T19:00:01,878 INFO  [ReadOnlyZKClient-127.0.0.1:2181@0x703f494c-SendThread(127.0.0.1:2181)] zookeeper.ClientCnxn: Session establishment complete on server localhost/127.0.0.1:2181, session id = 0x1001af67c2e0003, negotiated timeout = 40000
2024-04-26T19:00:01,881 INFO  [pool-100-thread-1] query.ConnectionQueryServicesImpl: er.connect(PhoenixDriver.java:229)
java.sql/java.sql.DriverManager.getConnection(DriverManager.java:677)
java.sql/java.sql.DriverManager.getConnection(DriverManager.java:189)
org.apache.phoenix.util.QueryUtil.getConnection(QueryUtil.java:433)
org.apache.phoenix.util.QueryUtil.getConnectionOnServer(QueryUtil.java:410)
org.apache.phoenix.util.QueryUtil.getConnectionOnServer(QueryUtil.java:391)
org.apache.phoenix.coprocessor.TaskRegionObserver$SelfHealingTask.run(TaskRegionObserver.java:164)
java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
java.base/java.lang.Thread.run(Thread.java:829)

2024-04-26T19:00:01,930 INFO  [pool-100-thread-1] connectionqueryservice.ConnectionQueryServicesMetricsManager: Created object for NoOp Connection query service metrics manager

@wendigo wendigo force-pushed the serafin/phoenix-5.2.0 branch from ab0c5fa to 3e460fe Compare April 26, 2024 19:17
@wendigo
Copy link
Contributor Author

wendigo commented Apr 26, 2024

Yep, startup issue. Added a wait condition and it passed for me locally couple of times.

@wendigo wendigo requested review from kokosing, ebyhr and ksobolew April 26, 2024 19:18
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Looks good.

Should we potentially update the docs to require Pheonix 5.2.0 as suggested in the sync meeting by @virajjasani and @stoty ?

@wendigo wendigo force-pushed the serafin/phoenix-5.2.0 branch from 1eda92e to 787aae2 Compare May 6, 2024 15:36
@wendigo
Copy link
Contributor Author

wendigo commented May 6, 2024

(just rebased)

@mosabua
Copy link
Member

mosabua commented May 6, 2024

probably because they are not run in containerized env.

I think that desired would be to use docker env for testing. It surely makes it easier to maintain and also tests are closer to test the real thing. Do you have a notion what would it take to use docker here?

Just to clarify @kokosing - Trino and this PR here is using docker containers. The Hbase and Phoenix project are not .. so if anything they can look at that on their end. This PR basically uncovered something new for them. So a successful collaboration..

@mosabua
Copy link
Member

mosabua commented May 6, 2024

I think the suggestion to merge this and improve aftewards is a good idea. The PR is already getting us a huge improvement of the prior state. Merging it potentially also allows others to send separate smaller PRs that will be faster to review and merge.

@wendigo
Copy link
Contributor Author

wendigo commented May 6, 2024

@mosabua right now these tests are flaky so there is no point in merging broken tests

@wendigo
Copy link
Contributor Author

wendigo commented May 6, 2024

If there is no workaround we would need to wait for the next HBase release basically.

@vincentpoon
Copy link
Member

I think the proposal here is to disable PhoenixTest for now and merge this, and come back later after next HBase patch release and re-enable the test.

@mosabua
Copy link
Member

mosabua commented May 6, 2024

Given the current situation .. @martint and @wendigo .. are we comfortable with disabling the test for now to avoid the flakiness and just making sure we follow up with the ticket @wendigo already created after the new release is out?

@wendigo wendigo force-pushed the serafin/phoenix-5.2.0 branch from 98740eb to 5dcbd67 Compare May 6, 2024 20:16
@lhofhansl
Copy link
Member

@vincentpoon Nice one. You still have it!

@wendigo
Copy link
Contributor Author

wendigo commented May 7, 2024

I'm fine with having only ConnectorTest but not PT for a time being @mosabua. Let's merge it and move forward.

@wendigo
Copy link
Contributor Author

wendigo commented May 7, 2024

@martint for a final approval

@wendigo wendigo requested a review from martint May 7, 2024 03:16
@martint
Copy link
Member

martint commented May 7, 2024

Agreed. Let's do that.

@mosabua mosabua merged commit 8687be7 into master May 7, 2024
106 checks passed
@mosabua mosabua deleted the serafin/phoenix-5.2.0 branch May 7, 2024 15:44
@mosabua
Copy link
Member

mosabua commented May 7, 2024

Thank you very much for the hugely successful collaboration @wendigo @lhofhansl @virajjasani @stoty @vincentpoon !

@github-actions github-actions bot added this to the 447 milestone May 7, 2024
@wendigo
Copy link
Contributor Author

wendigo commented May 7, 2024

@lhofhansl @vincentpoon @stoty why is the phoenix-client-lite-hbase-2.5 bringing hadoop and hbase dependency? I wanted to replace the embedded client with the phoenix + our own hbase + our own hadoop but it doesn't seem possible with the current artifacts.

@stoty
Copy link

stoty commented May 7, 2024

Hmm.
phoenix-client-lite doesn't actually have those dependecies, as those are shaded into it, so that sounds like a bug.

However, if you want to use your own Hbase and Hadoop, you should use the phoenix-mapreduce-byo-shaded-hbase artifact.

@stoty
Copy link

stoty commented May 8, 2024

If you are using unshaded hbase JARs, then use phoenix-server (the only difference between phoenix-server and phoenix-mapreduce-byo-shaded-hbase is whether it uses shaded or standard Protobuf 2.5.0. )

@stoty
Copy link

stoty commented May 8, 2024

I've just checked, and I cannot see phoenix-client-lite-hbase-2.5 depending on anything. Do you mean that it has dependencies shaded in ?

@wendigo
Copy link
Contributor Author

wendigo commented May 8, 2024

@stoty yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants