Skip to content

HIVE-28121. (2.3) Use direct SQL for transactional altering table parameter - #5204

Merged
sunchao merged 5 commits into
apache:branch-2.3from
pan3793:HIVE-28121-2.3
Apr 30, 2024
Merged

HIVE-28121. (2.3) Use direct SQL for transactional altering table parameter#5204
sunchao merged 5 commits into
apache:branch-2.3from
pan3793:HIVE-28121-2.3

Conversation

@pan3793

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Port #5129 to branch-2.3

Why are the changes needed?

This is a follow-up of HIVE-26882, including correctness fixes for some RDBMS.

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

UT

@Override
public long updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue)
throws MetaException, NoSuchObjectException {
final Table _table = table;

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this might be limitation of Java 7

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is ok

@lirui-apachelirui-apache left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1, thanks @pan3793 for taking care of this

@pan3793

Copy link
Copy Markdown
MemberAuthor

@pvary Could you please take a look at this one? AFAIK it's the last one before the next RC for 2.3.10

@pvarypvary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The backport seems ok.
I am a bit concerned about the failing tests, but if @sunchao says, that this is the current state of the tests on this branch, then we are ok to go

@pan3793

Copy link
Copy Markdown
MemberAuthor

kindly ping @sunchao, are we good to go?

@sunchao

Copy link
Copy Markdown
Member

Thanks @pan3793@lirui-apache and @pvary . It looks like this PR has more tests failing than the previous PR:

Screenshot 2024-04-23 at 8 57 41 PM

In the previous PR: #4892, there were only 27 tests failing (normally there would only be 26 tests but 1 was flaky in that PR).

Could you double check whether the test failures are related?

@sunchao

Copy link
Copy Markdown
Member

that this is the current state of the tests on this branch

Yes that's unfortunately the current state of branch-2.3. We do know a fixed set of tests are failing though and are using that as the baseline for testing against PRs.

@pan3793

Copy link
Copy Markdown
MemberAuthor

@sunchao based on the CI results of this PR branch, I think the failure tests should be flaky cases
http://ci.hive.apache.org/blue/organizations/jenkins/hive-precommit/activity?branch=PR-5204

@sunchao

Copy link
Copy Markdown
Member

OK, just re-triggered CI to confirm.

@sunchao

Copy link
Copy Markdown
Member

Hmm @pan3793 I tried the tests again for 2 times, and each time there were over 40 tests failing, which is quite more than the normal 26. Could you double-check whether there is anything we should fix here? Thanks.

@pan3793

Copy link
Copy Markdown
MemberAuthor

@sunchao the failed tests are not caused by this patch, I opened another dummy PR based on the latest branch-2.3, and got the same failures #5225

@sunchao

Copy link
Copy Markdown
Member

oops, thanks @pan3793 - let me check the commits in branch-2.3 and see which commit caused the regression

@sunchao
sunchao merged commit 10b3b03 into apache:branch-2.3Apr 30, 2024
@sunchao

Copy link
Copy Markdown
Member

Merged, thanks

@pan3793

Copy link
Copy Markdown
MemberAuthor

@sunchao can we have the next RC for 2.3.10?

@sunchao

Copy link
Copy Markdown
Member

Sorry for the delay @pan3793 . I just sent out RC1.

@pan3793
pan3793 deleted the HIVE-28121-2.3 branch May 11, 2024 06:29

long updateTableParam(Table table, String key, String expectedValue, String newValue) {
String queryText = String.format("UPDATE \"TABLE_PARAMS\" SET \"PARAM_VALUE\" = ? " +
"WHERE \"PARAM_KEY\" = ? AND \"PARAM_VALUE\" = ? AND \"TBL_ID\" IN " +

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Just for reference, we hit performance issues in our production environment on executing this SQL. Changing the IN to = would solve this issue, at least for MySQL 5.7

imageimage

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @pan3793 for reporting the issue. I agree it's better to use scalar subquery here. But since both 2.x and 3.x lines are EOL, and subquery is not used in 4.0, so I guess we won't have to do anything at the moment, right?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, the Hive 4.0 doesn't have such an issue. This is just a tip for users who want to port this patch to their internal legacy Hive version (2.3 or 3.x)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@pan3793@sunchao@lirui-apache@pvary@asf-ci-hive