suneet-s commented on a change in pull request #11078:
URL: https://github.com/apache/druid/pull/11078#discussion_r609135089
##########
File path: docs/configuration/index.md
##########
@@ -338,6 +338,8 @@ Coordinator and Overlord log changes to lookups, segment
load/drop rules, dynami
|--------|-----------|-------|
|`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit
history.|1 week|
|`druid.audit.manager.includePayloadAsDimensionInMetric`|Boolean flag on
whether to add `payload` column in service metric.|false|
+|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload,
in bytes, to store in Druid's metadata store audit table. If the size of audit
payload exceed this value, the audit log would be stored with a messaging
indicating that the payload was omitted instead. Setting maxPayloadSizeBytes to
-1 (default value) disables this check, meaning Druid will always store audit
payload regardless of it's size.|-1|
Review comment:
nit: This way we can avoid the update to the `.spelling` file
```suggestion
|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit
payload, in bytes, to store in Druid's metadata store audit table. If the size
of audit payload exceed this value, the audit log would be stored with a
messaging indicating that the payload was omitted instead. Setting
`maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid
will always store audit payload regardless of it's size.|-1|
```
##########
File path: core/src/main/java/org/apache/druid/common/config/ConfigSerde.java
##########
@@ -25,5 +25,6 @@
{
byte[] serialize(T obj);
String serializeToString(T obj);
+ String serializeSkipNullToString(T obj);
Review comment:
I don't understand this part of the code so well, could you add a
javadoc and talk about what the expectations of this function are and how
they're different from `serializeToString` above. Maybe javadocs will help
clarify this for me
##########
File path: core/src/main/java/org/apache/druid/audit/AuditManager.java
##########
@@ -28,6 +28,8 @@
public interface AuditManager
{
+ String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as the payload size
exceed limit configured by druid.audit.manager.maxPayloadSizeBytes";
Review comment:
javadocs please
##########
File path:
core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java
##########
@@ -50,6 +52,7 @@ public JacksonConfigManager(
this.configManager = configManager;
this.jsonMapper = jsonMapper;
this.auditManager = auditManager;
+ this.jsonMapperSkipNull =
jsonMapper.copy().setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
I think this object should be bound in the `JacksonModule` as a
singleton.
Even though JacksonConfigManager is a singleton, if this binding changes in
the future, we could inadvertently create an ObjectMapper per
JacksonConfigManager instead of a singleton that can be shared across the
process.
Otherwise, each time we create a new JacksonConfigMapper, we will create a
new ObjectMapper, even though, jsonMapper is a singleton.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
