Enrich MetastoreContext with ClientTags #22766
Conversation
konjac-h
requested review from
agrawalreetika,
a team,
hantangwangd,
vinothchandar,
7c00 and
shrinidhijoshi
as code owners
...tastore/src/main/java/com/facebook/presto/hive/metastore/SemiTransactionalHiveMetastore.java
Outdated
Show resolved
Hide resolved
...tastore/src/main/java/com/facebook/presto/hive/metastore/SemiTransactionalHiveMetastore.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/com/facebook/presto/hive/HiveQueryRunner.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergQueryRunner.java
Outdated
Show resolved
Hide resolved
| @@ -48,6 +51,11 @@ public Optional<String> getClientInfo() | |||
| return clientInfo; | |||
| } | |||
|
|
|||
| public Set<String> getClientTags() | |||
There was a problem hiding this comment.
exposes potentially mutable internal state. Might be risky in a security context.
| @@ -126,6 +132,11 @@ public Optional<String> getClientInfo() | |||
| return clientInfo; | |||
| } | |||
|
|
|||
| public Set<String> getClientTags() | |||
There was a problem hiding this comment.
On first review I didn't find a test for this new public method.
There was a problem hiding this comment.
It's a getter method. I haven't noticed other similar methods being tested either. In my opinion, testing this would be unnecessary repetition.
There was a problem hiding this comment.
It's not repetition. It's verification, and a prevention of regression.
I know our existing tests are lacking. That shouldn't stop us from doing better going forward.
There was a problem hiding this comment.
In my opinion, unit tests are there to test the behaviour of code, in a meaningful way, and getters/setters are only a means to an end. Writing unit tests for a getter method is proving to be a diversion for me.
|
Can we add specific in the commit title what we are enriching with ? |
konjac-h
force-pushed
the
enrich-metastore-context
branch
2 times, most recently
from
cfa77f5
to
4345b8d
Compare
konjac-h
changed the title
konjac-h
force-pushed
the
enrich-metastore-context
branch
from
4345b8d
to
c4a4134
Compare
Description
This PR enriches the metastore context with the client tags and corresponding constructor change
Motivation and Context
For Meta Internal, we want to enable certain features for metastore based on client tags. Hence, we introduce the client tags inside the metastore context
Impact
Metastore context will have another attribute
clientTagsTest Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.