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

[ZEPPELIN-5249]. Update to thrift 0.14.2 #4089

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

PrarthiJain
Copy link
Contributor

@PrarthiJain PrarthiJain commented Apr 8, 2021

What is this PR for?

• This PR is to upgrade thrift to 0.14.2

What type of PR is it?

• [Improvement]

Todos

• [ ] - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-5249

How should this be tested?

• CI pass

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@PrarthiJain
Copy link
Contributor Author

@prabhjyotsingh @VipinRathor @zjffdu, Could you please help in reviewing? Thanks.

@Reamer
Copy link
Contributor

Reamer commented Apr 8, 2021

Please remove your changes in the zeppelin-web submodule. I closed your JIRA ticket because it duplicates another one. Please change your PR title and link in your PR text.

Maybe we can remove the System.exit(0) for a clean shutdown of the Zeppelin interpreter. It would be nice if you could test this. See https://issues.apache.org/jira/browse/ZEPPELIN-5249

@Reamer
Copy link
Contributor

Reamer commented Apr 9, 2021

This PR also affects #4072, which still uses the old Thrift version.

@PrarthiJain
Copy link
Contributor Author

Thanks, @Reamer. Could you please help with the steps to test for a successful shutdown of the Zeppelin interpreter?

@Reamer
Copy link
Contributor

Reamer commented Apr 9, 2021

Thanks, @Reamer. Could you please help with the steps to test for a successful shutdown of the Zeppelin interpreter?

After #4072 has been merged, I can help you,

@PrarthiJain PrarthiJain changed the title [ZEPPELIN-5314]. Upgrade thrift to 0.14.1 [ZEPPELIN-5249]. Update to thrift 0.14.1 Apr 20, 2021
@PrarthiJain
Copy link
Contributor Author

@Reamer The checks are unexpectedly failing. Could you please help with this, along with the code review? Thank you.

@Reamer
Copy link
Contributor

Reamer commented May 3, 2021

CI is currently broken.
Please rebase after #4107 is merged.

@Reamer
Copy link
Contributor

Reamer commented May 5, 2021

#4107 was merged. Please rebase to current master.

@Reamer
Copy link
Contributor

Reamer commented May 11, 2021

Please take a look at the Cassandra interpreter. I think the following stack trace relates to your change.

WARN  [07:27:58,378][] org.apache.cassandra.db.SystemKeyspace@:getLocalHostId No host ID found, created 729402a1-a3b2-482a-aeb7-7238ab0f53ff (Note: This should happen exactly once per node). 
Exception (java.lang.NoClassDefFoundError) encountered during startup: org/apache/thrift/transport/TFramedTransport$Factory
java.lang.NoClassDefFoundError: org/apache/thrift/transport/TFramedTransport$Factory
	at org.apache.cassandra.service.CassandraDaemon.setup(CassandraDaemon.java:435)
	at org.apache.cassandra.service.CassandraDaemon.activate(CassandraDaemon.java:620)
	at org.cassandraunit.utils.EmbeddedCassandraServerHelper.lambda$startEmbeddedCassandra$1(EmbeddedCassandraServerHelper.java:152)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.ClassNotFoundException: org.apache.thrift.transport.TFramedTransport$Factory
	at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:419)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
	... 6 more

@zjffdu
Copy link
Contributor

zjffdu commented May 15, 2021

ping @PrarthiJain

@PrarthiJain
Copy link
Contributor Author

@Reamer @zjffdu @prabhjyotsingh. I have uploaded a new patch with thrift dependency addition. We can remove that later on the 4.0 version upgrade. Let me know your thoughts. Thanks.

@zjffdu
Copy link
Contributor

zjffdu commented May 17, 2021

It looks like cause spark/flink tests fail

prabhjyotsingh pushed a commit to prabhjyotsingh/zeppelin that referenced this pull request May 23, 2021
### What is this PR for?
• This PR is to upgrade thrift to 0.14.1

### What type of PR is it?
• [Improvement]

### Todos
• [ ] - Task

### What is the Jira issue?
• https://issues.apache.org/jira/browse/ZEPPELIN-5249

### How should this be tested?
• CI pass

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: PrarthiJain <[email protected]>

Closes apache#4089 from PrarthiJain/ZEPPELIN-5314 and squashes the following commits:

bae740d [PrarthiJain] add thrift dependency in zeppelin-cassandra
4e596e3 [PrarthiJain] ZEPPELIN-5249: Update to thrift 0.14.1
@PrarthiJain
Copy link
Contributor Author

@Reamer @zjffdu @prabhjyotsingh, The build failed with Could not resolve dependencies for...from/to maven-default-http-blocker (http://0.0.0.0/): Blocked mirror for repositories: .... Could you please help? Thanks.

@zjffdu
Copy link
Contributor

zjffdu commented May 26, 2021

@PrarthiJain I also notice that today, I am looking into this issue, will keep you updated

@zjffdu
Copy link
Contributor

zjffdu commented May 27, 2021

@PrarthiJain I made a hotfix for the CI, please rebase your PR and trigger the CI again.

@PrarthiJain
Copy link
Contributor Author

Thanks @zjffdu

@zjffdu
Copy link
Contributor

zjffdu commented Jun 9, 2021

@PrarthiJain Could you check the failed test ? AFAIK, frontend / test-selenium-with-spark-module-for-spark-2-3 and core / jdbcIntegrationTest-and-unit-test-of-Spark-2-4-with-Scala-2-11 has flaky tests, others should be successful.

@PrarthiJain
Copy link
Contributor Author

It seems the Alluxio Interpreter Test is failing, should be fixed with- #2900.

@zjffdu
Copy link
Contributor

zjffdu commented Jun 11, 2021

@PrarthiJain What about other failed tests ?

@PrarthiJain
Copy link
Contributor Author

@zjffdu Those were succeeded after re-triggering on my forked repo. However, I think I don't have permission to re-trigger and check the failed tests here. Do I need to upload a new patch again? Thanks.

@zjffdu
Copy link
Contributor

zjffdu commented Jun 11, 2021

@PrarthiJain You can do a force push to trigger the CI

Reamer referenced this pull request Jun 16, 2021
### What is this PR for?
This PR moves the exec code to a new class called `ExecRemoteInterpreterProcess`. This allows other `RemoteInterpreterProcess` classes to use the better code of `RemoteInterpreterManagedProcess`.

A soft shutdown has been implemented in the new `ExecRemoteInterpreterProcess` class.

### What type of PR is it?
- Improvement

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5225

### How should this be tested?
* CI

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Philipp Dallig <[email protected]>

Closes #4035 from Reamer/remote_interpreter_soft_shutdown and squashes the following commits:

c22df38 [Philipp Dallig] Correct LOGGER messages
8bebe6a [Philipp Dallig] RemoteInterpreterManagedProcess soft shutdown and abstraction

(cherry picked from commit 59bdb47)
Signed-off-by: Philipp Dallig <[email protected]>
@zjffdu
Copy link
Contributor

zjffdu commented Jun 16, 2021

@Reamer
Copy link
Contributor

Reamer commented Jun 18, 2021

Thrift 0.14.2 has been released.
This version also fixed a regression in the Java library.
Maybe we should wait for this version.

@zjffdu
Copy link
Contributor

zjffdu commented Jun 26, 2021

ping @PrarthiJain

Change-Id: I9714a5442da85404c7f776a5edb0aa05545da922
@PrarthiJain PrarthiJain changed the title [ZEPPELIN-5249]. Update to thrift 0.14.1 [ZEPPELIN-5249]. Update to thrift 0.14.2 Jul 12, 2021
Change-Id: Ia6a57e284de221a4c6ab629efe7b54a4f67f5e4c
Change-Id: I492f194b370733b3480662721e6b85048324ffe5
@PrarthiJain
Copy link
Contributor Author

@Reamer @zjffdu @prabhjyotsingh, I have updated the patch. Could you please help to review it? Thanks.

@Reamer
Copy link
Contributor

Reamer commented Jul 19, 2021

@Reamer @zjffdu @prabhjyotsingh, I have updated the patch. Could you please help to review it? Thanks.

I think your problems with updating the Thrift version are due to the ugly use of the interpreter shade jar.

<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>zeppelin-interpreter-shaded</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>zeppelin-interpreter</artifactId>
<version>${project.version}</version>
<scope>provided</scope>
</dependency>

I am trying to solve the problem in my interpreter_shade branch. . But the changes depend on an update of the shade-plugin (apache/maven-shade-plugin#100 or apache/maven-shade-plugin#105) because Intellji does not use the shading jars IDEA-93855 correctly.

@PrarthiJain
Copy link
Contributor Author

Thanks, @Reamer. Sure will wait till the next release of maven-shade-plugin

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

Successfully merging this pull request may close these issues.

3 participants