[thrift-audit-hook] Adding auditing for Thrift hooks #4
Conversation
john-bodley
force-pushed
the
johnbodley-thrift-audit-hook
branch
from
911d0aa
to
c5d956f
Compare
| // Will wait BASE_SLEEP * 2 ^ (attempt no.) between attempts. | ||
| private static final int BASE_SLEEP = 1; | ||
|
|
||
| public static String DB_USERNAME = "airbnb.reair.audit_log.db.username"; |
There was a problem hiding this comment.
For the rollout, we might want to log to a separate set of tables, so could you change these to a separate set of keys?
There was a problem hiding this comment.
Sure. Could you provide me with the temporary keys we should use?
There was a problem hiding this comment.
airbnb.reair.metastore.audit_log.db.username and likewise.
|
Great work! It looks good overall, but there may be some checkstyle issues though - can you try running |
| try { | ||
|
|
||
| // Table is invariant and thus arbitrary choice between old and new. | ||
| Table table = new Table( |
There was a problem hiding this comment.
event doesn't have the whole table object?
There was a problem hiding this comment.
Sadly no. I'm somewhat perplexed as to why.
I'll update the comment to explain this.
|
Also might be good to rename |
| * Audit logging for the metastore Thrift server. Comprises of a series of | ||
| * event listeners adding auditing to the various Trift event. | ||
| */ | ||
| public class MetastoreAuditLogHook extends MetaStoreEventListener { |
There was a problem hiding this comment.
Maybe this is better named a *Listener instead of a "*Hook"?
|
I ran |
|
Sorry, mean to say |
| private static final int BASE_SLEEP = 1; | ||
|
|
||
| public static String DB_USERNAME = | ||
| "airbnb.reair.metastoreaudit_log.db.username"; |
john-bodley
force-pushed
the
johnbodley-thrift-audit-hook
branch
from
04afcb7
to
716d809
Compare
| try { | ||
| Set<ReadEntity> readEntities = new HashSet<>(); | ||
| readEntities.add(new ReadEntity(new Table(event.getTable()))); | ||
| Set<WriteEntity> writeEntities = new HashSet<>(); |
There was a problem hiding this comment.
Current convention is that the dropped table is in outputs.
|
Chatted about comments offline - LGTM. Once you rebase and squash, I'll merge it in. |
|
Can you update the class name in |
john-bodley
force-pushed
the
johnbodley-thrift-audit-hook
branch
from
e02e49f
to
1d00ddb
Compare
This PR adds audit logging hooks to metastore command instantiated via the Thrift interface. Note some refactoring of the existing audit log hook was necessary by introducing a
SessionStateLiteobject to support Hive Operators not defined via theorg.apache.hadoop.hive.ql.plan.HiveOperationenum.The metastore Thrift hooks trigger via registered listeners where the various events are coerced into ReadEntity and WriteEntity objects.
Note I'm unsure why this is but the
AlterPartitionEventprovides only the old and new partitions without specifying a table which is somewhat perplexing as partitions are associated with tables. In theonAlterPartitioncallback a dummymetadata.Tableinstance with the bare necessities is created in order to instantiate ametadata.Partitionobject. This approach might not be the most elegant but it seems to capture the relevant information of the serialized object.Finally there's some additional cleanup of the
HiveOperationenum as well as leveraging thegetDbConnectionFactoryfor the AuditLogHookTest tests.to: @plypaul
Tested by running the following unit tests: