hutiefang76 commented on code in PR #4402:
URL: https://github.com/apache/amoro/pull/4402#discussion_r4220564497
##########
amoro-ams/src/main/java/org/apache/amoro/server/dashboard/ServerTableProperties.java:
##########
@@ -40,6 +40,9 @@ public class ServerTableProperties {
HIDDEN_EXPOSED.add(TableProperties.WRITE_DISTRIBUTION_HASH_PRIMARY_PARTITION);
HIDDEN_EXPOSED.add(TableProperties.WRITE_DISTRIBUTION_HASH_PRIMARY);
HIDDEN_EXPOSED.add(TableProperties.WRITE_DISTRIBUTION_HASH_PARTITION);
+ // Hide individual protected properties and the synthetic key produced by
reflecting
+ // the Set itself in the dashboard property browser.
+ HIDDEN_EXPOSED.addAll(TableProperties.WRITE_PROTECTED_PROPERTIES);
HIDDEN_EXPOSED.add(TableProperties.WRITE_PROTECTED_PROPERTIES.toString());
Review Comment:
I think we should keep it in this patch.
`addAll(WRITE_PROTECTED_PROPERTIES)` hides the individual property keys, while
this line hides the set's own `toString()` value.
`AmsUtil.getNotDeprecatedAndNotInternalStaticFields` still reflects the set
field, and `getUpgradeHiveTableProperties` uses that reflected value as an
option key.
I checked this with `TestTableControllerUpgradeProperties`: both tests pass
with the current code; removing this line makes the synthetic-set-key assertion
fail (2 tests, 1 failure). I restored the line after that check. We could
separately change the reflection path to exclude non-string fields, but the
line is still needed with the current path.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
