Repository navigation
HIVE-27855: Create external tables for Hive and Tez protologging events - #5036
Conversation
|
| while ((line = reader.readLine()) != null) { | ||
| for (String s: replacements.keySet()) { | ||
| if (line.contains(s)) { | ||
| line = line.replace(s, replacements.get(s)); |
There was a problem hiding this comment.
Could you please elaborate why it needs to replace? I guess it has a business logic part, but from review perspective hard to understand.
There was a problem hiding this comment.
The table locations for protologging events is populated from config variables in Hive and Tez. So, in the sql file, placeholders are used which are being replaced with the corresponding table locations from the config variables in Hive and Tez.
okumin
left a comment
okumin
left a comment
There was a problem hiding this comment.
I didn't know this nice feature of Hive and Tez.
I'm pretty sure that it is a good idea to make the tables queryable from Hive.
| } | ||
|
|
||
| Map<String, String> replacements = new HashMap<>(); | ||
| replacements.put("_REPLACE_WITH_QUERY_DATA_LOCATION_", "hdfs:///" + hiveProtoBaseDir); |
There was a problem hiding this comment.
I would say we may avoid hardcoding the prefix, i.e., following the logic using HIVE_PROTO_EVENTS_BASE_PATH.
https://github.com/apache/hive/blob/rel/release-4.0.0-beta-1/ql/src/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java#L67
As far as I tested, hive.hook.proto.base-directory can accept hdfs://path/to/proto or s3a://bucket/path/to/proto.
There was a problem hiding this comment.
Sorry for the late update as I was away due to personal issues. I have updated the files by working on your suggestions.
| replacements.put("_REPLACE_WITH_QUERY_DATA_LOCATION_", "hdfs:///" + hiveProtoBaseDir); | ||
| replacements.put("_REPLACE_WITH_APP_DATA_LOCATION_", "hdfs:///" + tezProtoBaseDir + "/app_data"); | ||
| replacements.put("_REPLACE_WITH_DAG_DATA_LOCATION_", "hdfs:///" + tezProtoBaseDir + "/dag_data"); | ||
| replacements.put("_REPLACE_WITH_DAG_META_LOCATION_", "hdfs:///" + tezProtoBaseDir + "/dag_meta"); |
There was a problem hiding this comment.
I presume we may also avoid hardcoding it for TEZ_HISTORY_LOGGING_PROTO_BASE_DIR.
There was a problem hiding this comment.
Sorry for the late update as I was away due to personal issues. I have updated the files by working on your suggestions.
3dff202 to
574b65d
Compare
|
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
b9a3000 to
d7d77ad
Compare
c0b4f58 to
ee511e9
Compare
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
ee511e9 to
dcbf0c9
Compare
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
dcbf0c9 to
d590adc
Compare
asf-ci-hive
added
tests pending
tests unstable
and removed
tests failed
tests pending
labels
d590adc to
3b7bc62
Compare
|
|
@KiranVelumuri, @okumin sorry for the late jump-in, I overlooked that PR. Also, could we add a qtest that generates the events and then output the |
Thank you @deniskuzZ for the review. The table location was based on the location of protologging events, which is in the sys db. This was done in order to standardise the table location for it to be consistent across versions. In my opinion, these tables' location in db is not meant to be changed by the end user, but instead query from it. The other option available was in INFORMATION_SCHEMA, but SYS db seems appropriate. I will work on adding the related qtest you mentioned. |
@KiranVelumuri, HIVE_PROTO_EVENTS_BASE_PATH is a Hive conf. Can't it be changed after the schema initialization? |
|
Yes, the protologging location for both Hive and Tez can be changed after schema initialization. In that case, the tables' location would have to be updated manually. |
in that case could we at least make the configs protected (i.e do not allow changes on the session level)? |
|
hi @KiranVelumuri, please ping me when you'll be ready for the final review. thanks! |
hi @deniskuzZ, I will be done with changes by this weekend and ping you then. |
|
|
@deniskuzZ sorry for the slight delay. I have made the requested changes and ready for review. |
|
hi @KiranVelumuri, sorry I missed the update, I'll merge it today |
5 participants
What changes were proposed in this pull request?
HiveProtoLoggingHook in Hive and ProtoHistoryLoggingService in Tez logs details, such as query execution, query plan, runtime statistics, serialised as protobuf files. This PR exposes these protobuf log files as external tables. This would be done during the information schema upgrade to the next Hive version, which is 4.0.0-beta-2 in this case.
Why are the changes needed?
By exposing the protobuf log files as external tables, it can be useful for debugging, performance tuning and auditing purposes.
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No
How was this patch tested?
This patch was tested by upgrading the information schema version and validating that the external tables have been created pointing to the correct specified locations.