You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/06/30 23:46:16 UTC
[GitHub] [hive] dengzhhu653 opened a new pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
dengzhhu653 opened a new pull request #2441:
URL: https://github.com/apache/hive/pull/2441
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r683067748
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
The `startFunctions` takes care of these:
1. API Metrics, which is duplicated with [PerfLogger](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/PerfLogger.java#L188-L207) in RetryingHMSHandler, this part cloud be elimated from the `startFunctions`.
2. Audit Logs, it is difficult to move these to a standalone function, as the log is related with the input of the method and the input varies among different methods. For example, we have two `startPartitionFunction` for the partition related methods:
```
private void startPartitionFunction(String function, String cat, String db, String tbl,
List<String> partVals) {
startFunction(function, " : tbl=" +
TableName.getQualified(cat, db, tbl) + "[" + join(partVals, ",") + "]");
}
private void startPartitionFunction(String function, String catName, String db, String tbl,
Map<String, String> partName) {
startFunction(function, " : tbl=" +
TableName.getQualified(catName, db, tbl) + "partition=" + partName);
}
```
In some cases, we also use only log the table name by using `startTableFunction`, like [get_partitions](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L5453)
These functions make the audit log of partitions different, another concern is that if we add/remove a method, the audit log should be changed elsewhere, this may be upsetting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r683080884
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -61,6 +66,7 @@ public Result(Object result, int numRetries) {
private final Configuration origConf; // base configuration
private final Configuration activeConf; // active configuration
+ private final List<MetaStoreEndFunctionListener> endFunctionListeners; // the end function listener
Review comment:
Thank you for the comments!
The `startFunctions` could be eliminated for the reasons listing above, leaving the `MetaStoreEndFunctionListener` on `endFunction` that should be taken care of. Introducing a `MetaStoreFunctionListener` makes things much better and straightforward. A user can monitor the input or output of the method, or implement his own audit logics.
This change also introduce some incompatibility in [MetaStoreEndFunctionContext](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreEndFunctionContext.java#L55-L57), the `getInputTableName` would return null for the old implemention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-995436584
Pros:
- More accurate and detail infos feeding into end listeners.
- Make the handler clean and break the monitor, audit into pieces.
Cons:
- Ruin backwards compatibility of MetaStoreEndFunctionContext, not sure how many users use it for tracking the handler's methods.
Hi @kgyrtkirk, what could I do to push it forward?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r683067748
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
The `startFunctions` takes care of these:
1. API Metrics, which is duplicated with [PerfLogger](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/PerfLogger.java#L188-L207) in RetryingHMSHandler, this part cloud be elimated from the `startFunctions`.
2. Audit Logs, it is difficult to move these to a standalone function, as the log is related with the input of the method. For example, we have two `startPartitionFunction` for the partition related methods:
```
private void startPartitionFunction(String function, String cat, String db, String tbl,
List<String> partVals) {
startFunction(function, " : tbl=" +
TableName.getQualified(cat, db, tbl) + "[" + join(partVals, ",") + "]");
}
private void startPartitionFunction(String function, String catName, String db, String tbl,
Map<String, String> partName) {
startFunction(function, " : tbl=" +
TableName.getQualified(catName, db, tbl) + "partition=" + partName);
}
```
In some cases, we also use only log the table name by using `startTableFunction`, like [get_partitions](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L5453)
These functions make the audit log of partitions different, another concern is that if we add/remove a method, the audit log should be changed elsewhere, this may be upsetting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] github-actions[bot] commented on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-974730554
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-1048784902
sure; sounds good to me!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-1048706430
> I think parts of this is already in via [HIVE-25892](https://issues.apache.org/jira/browse/HIVE-25892)
Yes, I want to split the changes into audit, monitor and some others for better reviewing, how do you think about that?
Thanks,
Zhihua Deng
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-892688659
@nrg4878 @kgyrtkirk any thought or comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r683067748
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
The `startFunctions` takes care of these:
1. API Metrics, which is duplicated with [PerfLogger](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/PerfLogger.java#L188-L207) in RetryingHMSHandler, this part cloud be eliminated from the `startFunctions`.
2. Audit Logs, it is difficult to move these to a standalone function, as the log is related with the input of the method and the input varies among different methods. For example, we have two `startPartitionFunction` for the partition related methods:
```
private void startPartitionFunction(String function, String cat, String db, String tbl,
List<String> partVals) {
startFunction(function, " : tbl=" +
TableName.getQualified(cat, db, tbl) + "[" + join(partVals, ",") + "]");
}
private void startPartitionFunction(String function, String catName, String db, String tbl,
Map<String, String> partName) {
startFunction(function, " : tbl=" +
TableName.getQualified(catName, db, tbl) + "partition=" + partName);
}
```
In some cases, we also use only log the table name by using `startTableFunction`, like [get_partitions](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L5453)
These functions make the audit log of partitions different, another concern is that if we add/remove a method, the audit log should be changed elsewhere, this may be upsetting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 edited a comment on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 edited a comment on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-892688659
Hi @nrg4878 @kgyrtkirk any thought or comments? thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-872670093
I'm not so sure if we could move MetaStoreEndFunctionListener to the InvocationHandler. @vihangk1 @kuczoram any thoughts or comments? Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r683080884
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -61,6 +66,7 @@ public Result(Object result, int numRetries) {
private final Configuration origConf; // base configuration
private final Configuration activeConf; // active configuration
+ private final List<MetaStoreEndFunctionListener> endFunctionListeners; // the end function listener
Review comment:
Thank you for the comments!
The `startFunctions` could be eliminated for the reasons listing above, leaving the `MetaStoreEndFunctionListener` on `endFunction` that should be taken care of, but introducing a `MetaStoreFunctionListener` makes things much better and straightforward. A user can monitor the input or output of the method, or implement his own audit logics.
This change also introduce some incompatibility in [MetaStoreEndFunctionContext](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreEndFunctionContext.java#L55-L57), the `getInputTableName` would return null for the old implemention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-892688659
@nrg4878 @kgyrtkirk any thought or comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r683067748
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
The `startFunctions` takes care of these:
1. API Metrics, which is duplicated with [PerfLogger](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/PerfLogger.java#L188-L207) in RetryingHMSHandler, this part cloud be elimated from the `startFunctions`.
2. Audit Logs, it is difficult to move these to a standalone function, as the log is related with the input of the method. For example, we have two `startPartitionFunction` for the partition related methods:
```
private void startPartitionFunction(String function, String cat, String db, String tbl,
List<String> partVals) {
startFunction(function, " : tbl=" +
TableName.getQualified(cat, db, tbl) + "[" + join(partVals, ",") + "]");
}
private void startPartitionFunction(String function, String catName, String db, String tbl,
Map<String, String> partName) {
startFunction(function, " : tbl=" +
TableName.getQualified(catName, db, tbl) + "partition=" + partName);
}
```
In some cases, we also use only log the table name by using `startTableFunction`, like [get_partitions](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L5453)
These functions make the audit log of partitions different, another concern is that if we add/remove a method, the audit log should be changed elsewhere, this may be upsetting.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
The `startFunctions` takes care of these:
1. API Metrics, which is duplicated with [PerfLogger](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/PerfLogger.java#L188-L207) in RetryingHMSHandler, this part cloud be elimated from the `startFunctions`.
2. Audit Logs, it is difficult to move these to a standalone function, as the log is related with the input of the method and the input varies among different methods. For example, we have two `startPartitionFunction` for the partition related methods:
```
private void startPartitionFunction(String function, String cat, String db, String tbl,
List<String> partVals) {
startFunction(function, " : tbl=" +
TableName.getQualified(cat, db, tbl) + "[" + join(partVals, ",") + "]");
}
private void startPartitionFunction(String function, String catName, String db, String tbl,
Map<String, String> partName) {
startFunction(function, " : tbl=" +
TableName.getQualified(catName, db, tbl) + "partition=" + partName);
}
```
In some cases, we also use only log the table name by using `startTableFunction`, like [get_partitions](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L5453)
These functions make the audit log of partitions different, another concern is that if we add/remove a method, the audit log should be changed elsewhere, this may be upsetting.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
The `startFunctions` takes care of these:
1. API Metrics, which is duplicated with [PerfLogger](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/PerfLogger.java#L188-L207) in RetryingHMSHandler, this part cloud be eliminated from the `startFunctions`.
2. Audit Logs, it is difficult to move these to a standalone function, as the log is related with the input of the method and the input varies among different methods. For example, we have two `startPartitionFunction` for the partition related methods:
```
private void startPartitionFunction(String function, String cat, String db, String tbl,
List<String> partVals) {
startFunction(function, " : tbl=" +
TableName.getQualified(cat, db, tbl) + "[" + join(partVals, ",") + "]");
}
private void startPartitionFunction(String function, String catName, String db, String tbl,
Map<String, String> partName) {
startFunction(function, " : tbl=" +
TableName.getQualified(catName, db, tbl) + "partition=" + partName);
}
```
In some cases, we also use only log the table name by using `startTableFunction`, like [get_partitions](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L5453)
These functions make the audit log of partitions different, another concern is that if we add/remove a method, the audit log should be changed elsewhere, this may be upsetting.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -61,6 +66,7 @@ public Result(Object result, int numRetries) {
private final Configuration origConf; // base configuration
private final Configuration activeConf; // active configuration
+ private final List<MetaStoreEndFunctionListener> endFunctionListeners; // the end function listener
Review comment:
Thank you for the comments!
The `startFunctions` could be eliminated for the reasons listing above, leaving the `MetaStoreEndFunctionListener` on `endFunction` that should be taken care of, but introducing a `MetaStoreFunctionListener` makes things much better and straightforward. A user can monitor the input or output of the method, or implement his own audit logics.
This change also introduce some incompatibility in [MetaStoreEndFunctionContext](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreEndFunctionContext.java#L55-L57), the `getInputTableName` would return null for the old implemention.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -61,6 +66,7 @@ public Result(Object result, int numRetries) {
private final Configuration origConf; // base configuration
private final Configuration activeConf; // active configuration
+ private final List<MetaStoreEndFunctionListener> endFunctionListeners; // the end function listener
Review comment:
Thank you for the comments!
The `startFunctions` could be eliminated for the reasons listing above, leaving the `MetaStoreEndFunctionListener` on `endFunction` that should be taken care of. Introducing a `MetaStoreFunctionListener` makes things much better and straightforward. A user can monitor the input or output of the method, or implement his own audit logics.
This change also introduce some incompatibility in [MetaStoreEndFunctionContext](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreEndFunctionContext.java#L55-L57), the `getInputTableName` would return null for the old implemention.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
The `startFunctions` takes care of these:
1. API Metrics, which is duplicated with [PerfLogger](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/PerfLogger.java#L188-L207) in RetryingHMSHandler, this part cloud be eliminated from the `startFunctions`.
2. Audit Logs, it is difficult to move these to a standalone function, as the log is related with the input of the method and the input varies among different methods. For example, we have two `startPartitionFunction` for the partition related methods:
```
private void startPartitionFunction(String function, String cat, String db, String tbl,
List<String> partVals) {
startFunction(function, " : tbl=" +
TableName.getQualified(cat, db, tbl) + "[" + join(partVals, ",") + "]");
}
private void startPartitionFunction(String function, String catName, String db, String tbl,
Map<String, String> partName) {
startFunction(function, " : tbl=" +
TableName.getQualified(catName, db, tbl) + "partition=" + partName);
}
```
In some cases, we only log the table name by using `startTableFunction`, like [get_partitions](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L5453)
These functions make the audit log of partitions different, another concern is that if we add/remove a method, the audit log should be changed elsewhere, this may be upsetting.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -61,6 +66,7 @@ public Result(Object result, int numRetries) {
private final Configuration origConf; // base configuration
private final Configuration activeConf; // active configuration
+ private final List<MetaStoreEndFunctionListener> endFunctionListeners; // the end function listener
Review comment:
Thank you for the comments!
The `startFunctions` could be eliminated for the reasons listing above, leaving the `MetaStoreEndFunctionListener` on `endFunction` that should be taken care of. Introducing a `MetaStoreFunctionListener` makes things much straightforward, but we can get the context of the current method from `MetaStoreEndFunctionContext`, making it usless to have an extra start function listener.
This change also introduce some incompatibility in [MetaStoreEndFunctionContext](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreEndFunctionContext.java#L55-L57), the `getInputTableName` would return null for the old implemention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 edited a comment on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 edited a comment on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-995436584
Pros:
- More accurate and detail infos feeding into end listeners.
- Make the handler clean and break the monitor, audit into pieces.
Cons:
- Ruin backwards compatibility of MetaStoreEndFunctionContext, not sure how many users use it for tracking the handler's methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r754179495
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -913,74 +900,6 @@ private static void logAndAudit(final String m) {
logAuditEvent(m);
}
- private String startFunction(String function, String extraLogInfo) {
- incrementCounter(function);
- logAndAudit((getThreadLocalIpAddress() == null ? "" : "source:" + getThreadLocalIpAddress() + " ") +
- function + extraLogInfo);
- com.codahale.metrics.Timer timer =
- Metrics.getOrCreateTimer(MetricsConstants.API_PREFIX + function);
Review comment:
yes, we have double counted the metrics of api calling by the startFunction and the PerfLogger, e.g, the total call count and the active count of the api.
##########
File path: standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEndFunctionListener.java
##########
@@ -83,7 +86,7 @@ public void testEndFunctionListener() throws Exception {
listSize = DummyEndFunctionListener.funcNameList.size();
String func_name = DummyEndFunctionListener.funcNameList.get(listSize-1);
MetaStoreEndFunctionContext context = DummyEndFunctionListener.contextList.get(listSize-1);
- assertEquals(func_name,"get_database");
+ assertEquals(func_name,"get_database_req");
Review comment:
Thanks a lot for the feedback! you are right, this is where may break compatibility, including the MetaStoreEndFunctionContext(we cannot retrieve inputTableName by method `getInputTableName()` any more). I not so sure if we can make such changes though this can help clean some codes...
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -4059,7 +3806,7 @@ public Partition append_partition_with_environment_context(final String dbName,
String location;
PartValEqWrapperLite(Partition partition) {
- this.values = partition.isSetValues()? partition.getValues() : null;
+ this.values = partition.isSetValues()? new ArrayList<>(partition.getValues()) : null;
Review comment:
Not a bug really, just to clean up the codes. The following pieces of codes can be simply treated as `lhsValues.equals(rhsValues)` by `equals` of an ArrayList.
if (lhsValues.size() != rhsValues.size()) {
return false;
}
for (int i=0; i<lhsValues.size(); ++i) {
String lhsValue = lhsValues.get(i);
String rhsValue = rhsValues.get(i);
if ((lhsValue == null && rhsValue != null)
|| (lhsValue != null && !lhsValue.equals(rhsValue))) {
return false;
}
}
return true;`
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreAuditLogBuilder.java
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.metastore;
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.hadoop.hive.common.TableName;
+
+import static org.apache.commons.lang3.StringUtils.join;
+
+/**
+ * Generate the audit log in a builder manner.
+ */
+public class MetaStoreAuditLogBuilder {
+ // the function
+ private final String functionName;
+ private final StringBuilder builder;
+
+ private MetaStoreAuditLogBuilder(String functionName) {
+ this.functionName = functionName;
+ this.builder = new StringBuilder();
+ }
+
+ public static MetaStoreAuditLogBuilder functionName(String functionName) {
+ MetaStoreAuditLogBuilder builder = new MetaStoreAuditLogBuilder(functionName);
+ return builder;
+ }
+
+ public MetaStoreAuditLogBuilder connectorName(String connectorName) {
+ builder.append("connector=").append(connectorName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder catalogName(String catalogName) {
+ builder.append("catName=").append(catalogName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder dbName(String dbName) {
+ builder.append("db=").append(dbName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder tableName(String tableName) {
+ builder.append("tbl=").append(tableName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder packageName(String packageName) {
+ builder.append("package=").append(packageName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder typeName(String typeName) {
+ builder.append("type=").append(typeName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder pattern(String pattern) {
+ builder.append("pat=").append(pattern).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder extraInfo(String extraInfo) {
Review comment:
Make sense, let me take a look. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 edited a comment on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 edited a comment on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-892688659
Hi @nrg4878 @kgyrtkirk any thought or comments? thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 closed pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 closed pull request #2441:
URL: https://github.com/apache/hive/pull/2441
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-1048605634
I think parts of this is already in via HIVE-25892
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r682695432
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -61,6 +66,7 @@ public Result(Object result, int numRetries) {
private final Configuration origConf; // base configuration
private final Configuration activeConf; // active configuration
+ private final List<MetaStoreEndFunctionListener> endFunctionListeners; // the end function listener
Review comment:
didn't know we had something like this :)
but having only `MetaStoreEndFunctionListener` seems a bit odd;
how about:
* introduce a `MetaStoreFunctionListener`
* keep the `MetaStoreEndFunctionListener` interface but extend the new interface with default implementation of `startFunction`
* move the old logging/counting stuff into a `MetaStoreFunctionListener`
just an idea; let me know what you think!
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
I think we could place the `startFunctions` here as well - now we have 1 part here and the other there...
I don't think we will break much things if we do that - or do you think that could cause some trouble?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r683080884
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -61,6 +66,7 @@ public Result(Object result, int numRetries) {
private final Configuration origConf; // base configuration
private final Configuration activeConf; // active configuration
+ private final List<MetaStoreEndFunctionListener> endFunctionListeners; // the end function listener
Review comment:
Thank you for the comments!
The `startFunctions` could be eliminated for the reasons listing above, leaving the `MetaStoreEndFunctionListener` on `endFunction` that should be taken care of. Introducing a `MetaStoreFunctionListener` makes things much straightforward, but we can get the context of the current method from `MetaStoreEndFunctionContext`, making it usless to have an extra start function listener.
This change also introduce some incompatibility in [MetaStoreEndFunctionContext](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreEndFunctionContext.java#L55-L57), the `getInputTableName` would return null for the old implemention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r683067748
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
The `startFunctions` takes care of these:
1. API Metrics, which is duplicated with [PerfLogger](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/PerfLogger.java#L188-L207) in RetryingHMSHandler, this part cloud be eliminated from the `startFunctions`.
2. Audit Logs, it is difficult to move these to a standalone function, as the log is related with the input of the method and the input varies among different methods. For example, we have two `startPartitionFunction` for the partition related methods:
```
private void startPartitionFunction(String function, String cat, String db, String tbl,
List<String> partVals) {
startFunction(function, " : tbl=" +
TableName.getQualified(cat, db, tbl) + "[" + join(partVals, ",") + "]");
}
private void startPartitionFunction(String function, String catName, String db, String tbl,
Map<String, String> partName) {
startFunction(function, " : tbl=" +
TableName.getQualified(catName, db, tbl) + "partition=" + partName);
}
```
In some cases, we only log the table name by using `startTableFunction`, like [get_partitions](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L5453)
These functions make the audit log of partitions different, another concern is that if we add/remove a method, the audit log should be changed elsewhere, this may be upsetting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r754132199
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/PerfLogger.java
##########
@@ -186,24 +187,19 @@ public Long getDuration(String method) {
private transient Timer.Context totalApiCallsTimerContext = null;
private void beginMetrics(String method) {
- Timer timer = Metrics.getOrCreateTimer(MetricsConstants.API_PREFIX + method);
- if (timer != null) {
- timerContexts.put(method, timer.time());
- }
- timer = Metrics.getOrCreateTimer(MetricsConstants.TOTAL_API_CALLS);
- if (timer != null) {
- totalApiCallsTimerContext = timer.time();
- }
+ Optional.ofNullable(Metrics.getOrCreateTimer(MetricsConstants.API_PREFIX + method))
+ .ifPresent(timer -> timerContexts.put(method, timer.time()));
+ Optional.ofNullable(Metrics.getOrCreateTimer(MetricsConstants.TOTAL_API_CALLS))
+ .ifPresent(timer -> {totalApiCallsTimerContext = timer.time();});
+ Optional.ofNullable(Metrics.getOrCreateCounter(MetricsConstants.ACTIVE_CALLS + method))
+ .ifPresent(counter -> counter.inc());
Review comment:
I'm usually not agains changes like this - however this seems like a pretty hot method:
the old implementation have only created 1 new string objects which will be garbage collected - however the new method creates 3 sets of `Nullable` objects and some function objects to make the call...
can we add the `ACTIVE_CALLS` stuff the boring way?
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreAuditLogBuilder.java
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.metastore;
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.hadoop.hive.common.TableName;
+
+import static org.apache.commons.lang3.StringUtils.join;
+
+/**
+ * Generate the audit log in a builder manner.
+ */
+public class MetaStoreAuditLogBuilder {
+ // the function
+ private final String functionName;
+ private final StringBuilder builder;
+
+ private MetaStoreAuditLogBuilder(String functionName) {
+ this.functionName = functionName;
+ this.builder = new StringBuilder();
+ }
+
+ public static MetaStoreAuditLogBuilder functionName(String functionName) {
+ MetaStoreAuditLogBuilder builder = new MetaStoreAuditLogBuilder(functionName);
+ return builder;
+ }
+
+ public MetaStoreAuditLogBuilder connectorName(String connectorName) {
+ builder.append("connector=").append(connectorName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder catalogName(String catalogName) {
+ builder.append("catName=").append(catalogName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder dbName(String dbName) {
+ builder.append("db=").append(dbName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder tableName(String tableName) {
+ builder.append("tbl=").append(tableName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder packageName(String packageName) {
+ builder.append("package=").append(packageName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder typeName(String typeName) {
+ builder.append("type=").append(typeName).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder pattern(String pattern) {
+ builder.append("pat=").append(pattern).append(" ");
+ return this;
+ }
+
+ public MetaStoreAuditLogBuilder extraInfo(String extraInfo) {
Review comment:
all the callsites of this method look like:
```
" some=" + value
```
they not only expose the internal "key=value" strucutre; but they should also add and extra whitespace.
I think it would be better to add arguments like `key, value` - and let the builder take up some responsibility.
one more thing: `extraInfo` is pasted directly into the output builder; meanwhile for other methods the `key=` is written by this class.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -4059,7 +3806,7 @@ public Partition append_partition_with_environment_context(final String dbName,
String location;
PartValEqWrapperLite(Partition partition) {
- this.values = partition.isSetValues()? partition.getValues() : null;
+ this.values = partition.isSetValues()? new ArrayList<>(partition.getValues()) : null;
Review comment:
is this a bugfix? if yes - could you open a separate jira to submit it?
##########
File path: standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEndFunctionListener.java
##########
@@ -83,7 +86,7 @@ public void testEndFunctionListener() throws Exception {
listSize = DummyEndFunctionListener.funcNameList.size();
String func_name = DummyEndFunctionListener.funcNameList.get(listSize-1);
MetaStoreEndFunctionContext context = DummyEndFunctionListener.contextList.get(listSize-1);
- assertEquals(func_name,"get_database");
+ assertEquals(func_name,"get_database_req");
Review comment:
after this change it seems like `EndFunctionListers` will get a bit different data - is that true?
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -913,74 +900,6 @@ private static void logAndAudit(final String m) {
logAuditEvent(m);
}
- private String startFunction(String function, String extraLogInfo) {
- incrementCounter(function);
- logAndAudit((getThreadLocalIpAddress() == null ? "" : "source:" + getThreadLocalIpAddress() + " ") +
- function + extraLogInfo);
- com.codahale.metrics.Timer timer =
- Metrics.getOrCreateTimer(MetricsConstants.API_PREFIX + function);
Review comment:
I wonder if its an error if we already have a timer running for a method already - I think in normal cases we may have only one api call running in a single connection...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] github-actions[bot] commented on pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #2441:
URL: https://github.com/apache/hive/pull/2441#issuecomment-974730554
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #2441:
URL: https://github.com/apache/hive/pull/2441#discussion_r682695432
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -61,6 +66,7 @@ public Result(Object result, int numRetries) {
private final Configuration origConf; // base configuration
private final Configuration activeConf; // active configuration
+ private final List<MetaStoreEndFunctionListener> endFunctionListeners; // the end function listener
Review comment:
didn't know we had something like this :)
but having only `MetaStoreEndFunctionListener` seems a bit odd;
how about:
* introduce a `MetaStoreFunctionListener`
* keep the `MetaStoreEndFunctionListener` interface but extend the new interface with default implementation of `startFunction`
* move the old logging/counting stuff into a `MetaStoreFunctionListener`
just an idea; let me know what you think!
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
##########
@@ -202,6 +215,8 @@ public Result invokeInternal(final Object proxy, final Method method, final Obje
LOG.error(ExceptionUtils.getStackTrace(e.getCause()));
throw e.getCause();
}
+ } finally {
+ endFunction(method, object, ex, args);
Review comment:
I think we could place the `startFunctions` here as well - now we have 1 part here and the other there...
I don't think we will break much things if we do that - or do you think that could cause some trouble?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 closed pull request #2441: HIVE-25048: Improve the start/end functions in HMSHandler
Posted by GitBox <gi...@apache.org>.
dengzhhu653 closed pull request #2441:
URL: https://github.com/apache/hive/pull/2441
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org