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.

Sign up for GitHub

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

Jump to bottom

HIVE-28194: Regenerate HS2 thrift port in TestRetryingThriftCLIServiceClient #5205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

HIVE-28194: Regenerate HS2 thrift port in TestRetryingThriftCLIServiceClient #5205

wants to merge 1 commit into from

Conversation

armitage420
Copy link
Contributor

What changes were proposed in this pull request?

Hive thrift port was hardcoded to 15000, hence port regeneration code was added here

Why are the changes needed?

Testcase can fail if the port is occupied by another test in testsplit pipeline

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

Local machine

deniskuzZ
deniskuzZ reviewed May 27, 2024
View reviewed changes
service/src/test/org/apache/hive/service/server/TestHS2HttpServer.java Outdated
private static String metastorePasswd = "61ecbc41cdae3e6b32712a06c73606fa"; //random md5
private static Integer webUIPort = null;
private static String apiBaseURL = null;


@BeforeClass
public static void beforeTests() throws Exception {
webUIPort = MetaStoreTestUtils.findFreePortExcepting(
ThriftCLITestUtils.setUpBeforeClass();
Copy link
Member

@deniskuzZ deniskuzZ May 27, 2024 •
edited

Choose a reason for hiding this comment

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

why do you need to call this explicitly, it has @BeforeClass annotation?

deniskuzZ
deniskuzZ reviewed May 27, 2024
View reviewed changes
service/src/test/org/apache/hive/service/cli/TestRetryingThriftCLIServiceClient.java Outdated

@BeforeClass
public static void setUpBeforeClass() throws Exception{
ThriftCLITestUtils.setUpBeforeClass();
Copy link
Member

Choose a reason for hiding this comment

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

why to override it? it's same as parent

deniskuzZ
deniskuzZ reviewed May 27, 2024
View reviewed changes
...-unit/src/test/java/org/apache/hive/service/cli/thrift/TestThriftHttpCLIServiceFeatures.java Outdated
@@ -78,7 +78,7 @@
* classes instead of jdbc.
*/

public class TestThriftHttpCLIServiceFeatures {
public class TestThriftHttpCLIServiceFeatures extends ThriftCLITestUtils {
Copy link
Member

Choose a reason for hiding this comment

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

nit: space

deniskuzZ
deniskuzZ reviewed May 27, 2024
View reviewed changes
...-unit/src/test/java/org/apache/hive/service/cli/thrift/TestThriftHttpCLIServiceFeatures.java Outdated
@@ -138,16 +138,15 @@ public boolean hasCookieHeader() {
@BeforeClass
public static void setUpBeforeClass() throws Exception {
// Set up the base class
ThriftCLIServiceTest.setUpBeforeClass();
ThriftCLITestUtils.setUpBeforeClass();
Copy link
Member

Choose a reason for hiding this comment

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

doesn't parent method gets invoked before that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this particular class, we will have to do an explicit call to initialise hs2, and other members as the current setUpBeforeClass() method overrides it's parent method.

Copy link
Member

@deniskuzZ deniskuzZ May 28, 2024 •
edited

Choose a reason for hiding this comment

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

it's static, it can't override the parent.
why do you annotate @BeforeClass setUpBeforeClass in ThriftCLITestUtils then? Maybe you should remove this and don't extend from ThriftCLITestUtils class, use it as Util

Copy link
Member

@deniskuzZ deniskuzZ May 28, 2024 •
edited

Choose a reason for hiding this comment

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

see how the same is done in ITestDbTxnManager.setupDb that extends DbTxnManagerEndToEndTestBase

deniskuzZ
deniskuzZ reviewed May 27, 2024
View reviewed changes
...e-unit/src/test/java/org/apache/hive/service/cli/thrift/TestThriftCliServiceMessageSize.java Outdated
port = MetaStoreTestUtils.findFreePort();
hiveServer2 = new HiveServer2();
hiveConf = new HiveConf();
ThriftCLITestUtils.setUpBeforeClass();
Copy link
Member

Choose a reason for hiding this comment

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

same thing, why to override?

deniskuzZ
deniskuzZ reviewed May 27, 2024
View reviewed changes
Copy link
Member

@deniskuzZ deniskuzZ left a comment •
edited

Choose a reason for hiding this comment

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

in general LGTM, some questions on parent setUpBeforeClass() usage. I would expect next order:

  1. ThriftCLIServiceTest.setUpBeforeClass()
  2. Test Class .setUpBeforeClass()

no need for explicit calls

deniskuzZ
deniskuzZ reviewed May 28, 2024
View reviewed changes
service/src/test/org/apache/hive/service/server/TestHS2HttpServer.java
if (!hs2Started) {
throw (hs2Exception);
}
startHiveServer2WithConf(hiveConf);
Copy link
Member

Choose a reason for hiding this comment

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

are we starting 2 HS2s here? one in base class setup and 1 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are initialising hs2, port, and hiveconf in base class. hs2 is started only in testclasses. Hence it is not started twice

deniskuzZ
deniskuzZ reviewed May 28, 2024
View reviewed changes
service/src/test/org/apache/hive/service/cli/thrift/ThriftCLITestUtils.java

import org.junit.BeforeClass;

public abstract class ThriftCLITestUtils {
Copy link
Member

Choose a reason for hiding this comment

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

i am not sure if we should extend from it or just use it as Utils class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the setup base class as static. Keeping this class as abstract as ThriftCLIServiceTest class followed by other test classes inherit the variables declared here. And the code was getting pretty cluttered on doing so.

I have also removed the explicit calls of member variables.

deniskuzZ
deniskuzZ reviewed May 28, 2024
View reviewed changes
service/src/test/org/apache/hive/service/cli/thrift/ThriftCLITestUtils.java Outdated
hiveConf = new HiveConf();
}

protected static void startHiveServer2WithConf(HiveConf hiveConf) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

what is that 'protected static'? you can't override static methods

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@armitage420
Copy link
Contributor Author

@deniskuzZ I have made the required changes, I hope you could review the PR when possible!

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

@deniskuzZ deniskuzZ deniskuzZ left review comments

Assignees
No one assigned
Labels
tests passed
Projects
None yet
Milestone
No milestone
3 participants
@armitage420 @deniskuzZ @asf-ci-hive