Skip to content

Conversation

@abstractdog
Copy link
Contributor

@abstractdog abstractdog commented Jan 26, 2024

What changes were proposed in this pull request?

Move TestDbTxnManager2.checkLock to TestTxnDbUtil.checkLock

Why are the changes needed?

Described on jira.

Does this PR introduce any user-facing change?

No.

Is the change a dependency upgrade?

No.

How was this patch tested?

Unit tests should pass.

@InvisibleProgrammer
Copy link
Contributor

I have concerns about making hive-exec to depend on standalone-metastore, for two reasons:
Firstly, we already too complex module dependencies in the project. It just makes it more complex.
Secondly, standalone-metastore should be really standalone or as close to standalone as it is possible. I don't know if we have ongoing plans to move standalone metastore out of Hive repository but I can see it as a long-term goal.

What about putting the method into some similar purpose util class in the hive-exec module? It is already referenced in hive-it-unit.
In that special case, I would be fine with copy-pasting the method to hive-exec only so that standalone-metastore can kept independently from hive-exec.

@abstractdog
Copy link
Contributor Author

I have concerns about making hive-exec to depend on standalone-metastore, for two reasons: Firstly, we already too complex module dependencies in the project. It just makes it more complex. Secondly, standalone-metastore should be really standalone or as close to standalone as it is possible. I don't know if we have ongoing plans to move standalone metastore out of Hive repository but I can see it as a long-term goal.

What about putting the method into some similar purpose util class in the hive-exec module? It is already referenced in hive-it-unit. In that special case, I would be fine with copy-pasting the method to hive-exec only so that standalone-metastore can kept independently from hive-exec.

thanks for the comments!
as a matter of fact we already have a dependency on standalone-metastore-server in the same pom.xml

Secondly, standalone-metastore should be really standalone or as close to standalone as it is possible. 

it's standalone in a sense that it doesn't depend on hive-exec, but not the other way: if you look at ql/pom.xml, hive really depends on hive metastore, and that's by design: I believe being standalone doesn't mean that others cannot depend on you, and that's what's reflected in another poms in the hive code as well

Copy link
Contributor

@InvisibleProgrammer InvisibleProgrammer left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

Copy link
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

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

LGTM +1

@abstractdog abstractdog merged commit 3e4d464 into apache:master Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants