[jihoonson]

LocalTaskActionClient adds an audit log whenever it is submitted if the action should be audited. Here is the code snippet.

      try {
        final long auditLogStartTime = System.currentTimeMillis();
        storage.addAuditLog(task, taskAction);
        emitTimerMetric("task/action/log/time", System.currentTimeMillis() - auditLogStartTime);
      }

The addAuditLog() method internally inserts the byte-serialized taskAction to the metastore.

This can make actions failed if they are too large. One of the example of such large actions can be found in batch ingestion: if input data is very large, there can be a lot of segments to be published. HadoopIndexTask currently workarounds this issue by publishing segments per interval like below, but this is a temporal solution and there should be the same issue if there are too many segments per interval.

  public void publishSegments(Iterable<DataSegment> segments) throws IOException
  {
    // Request segment pushes for each set
    final Multimap<Interval, DataSegment> segmentMultimap = Multimaps.index(
        segments,
        new Function<DataSegment, Interval>()
        {
          @Override
          public Interval apply(DataSegment segment)
          {
            return segment.getInterval();
          }
        }
    );
    for (final Collection<DataSegment> segmentCollection : segmentMultimap.asMap().values()) {
      getTaskActionClient().submit(new SegmentInsertAction(ImmutableSet.copyOf(segmentCollection)));
    }
  }

This PR is to allow multiple rows for the same action, so that we can avoid such failures.

---

This change is 

[drcrallen]

Does this require SQL upgrades on metadata storage?

[jihoonson]

Oh, yes. I added labels.

[drcrallen]

I don't think you can roll back to a prior version in this proposed implementation. Can you change it so that you can roll back and forward across a version with this patch?

[jihoonson]

Hmm, probably it's possible. But the tasklogs table is only used for a single API to get segments generated by a task (https://github.com/druid-io/druid/blob/master/docs/content/design/indexing-service.md#submitting-tasks-and-querying-task-status). I think supporting rolling back might be too complicated for this single usage. Maybe it makes sense to note this incompatible change properly and users should drop and create tasklogs table if they want to roll back in the release notes. What do you think?

[gianm]

I think it'd be best to create a new table.

It's annoying, but, metadata schema changes are super disruptive, so I think it's worth the extra work.

[jihoonson]

That's a nice solution. I'll update PR soon.

[gianm]

Thanks. If you do that, then we can remove the "incompatible" tag. And let's all take a bit to consider the schema of the new table, so we don't need to do this again :)

[jihoonson]

Updated to use a new table.

I also changed the configuration name for the task audit logs from druid.metadata.storage.tables.taskLogTable to druid.metadata.storage.tables.taskActionsTable to avoid a case that users accidentally use the same name for the new table with the old one which will cause errors. I think this should be still labeled as 'incompatible' because of this configuration change.

[jon-wei]

Will the entry_id ever be null? Maybe it can have NOT NULL in the schema as well

[jon-wei]

I wonder if this is too big for a log message, given that these payloads can be several MB

[jon-wei]

It does seem a little weird to me to use the metadata store to hold split chunks of a large binary payload across multiple rows.

This would be a bigger design change, but I wonder if it would be better to keep the task action audit logs in deep storage instead, in a per-task file keyed by task ID.

It doesn't look like Druid uses this audit table internally for any internal decision making.

[jihoonson]

This would be a bigger design change, but I wonder if it would be better to keep the task action audit logs in deep storage instead, in a per-task file keyed by task ID.

That's a good point. It might be a good idea if we need to store audit logs.
But, even before thinking about this design change, I wonder what's the use case of http://<OVERLORD_IP>:<port>/druid/indexer/v1/task/{taskId}/segments API which is the only place reading the audit logs to get a list of segments inserted by a task. Why do users need to get a list of segments generated by a task? One use case I can imagine is that some users may want to revert changes made by a task, but I think it would be better to provide an API for reverting rather than returning a list of segments generated by a task.

@gianm any thoughts?

[gianm]

That's a good point. It might be a good idea if we need to store audit logs.
But, even before thinking about this design change, I wonder what's the use case of http://<OVERLORD_IP>:/druid/indexer/v1/task/{taskId}/segments API which is the only place reading the audit logs to get a list of segments inserted by a task. Why do users need to get a list of segments generated by a task? One use case I can imagine is that some users may want to revert changes made by a task, but I think it would be better to provide an API for reverting rather than returning a list of segments generated by a task.

Honestly, the only reason that API exists is because when I was originally building the task action system, I thought it might be cool to see what segments a task created. I didn't have a specific use case in mind. I am not sure if anyone in the community has built anything interesting around it since then or not.

This would be a bigger design change, but I wonder if it would be better to keep the task action audit logs in deep storage instead, in a per-task file keyed by task ID.

Storing audit logs in deep storage (like task reports) seems like a better idea to me than the metadata store. I suppose each one would need to be its own file. I would support moving them there, and maybe making them optional. I think they should be off by default -- they aren't too useful and they'll create a lot of extra files on deep storage (which people sometimes try to minimize, especially if they're on a system like hdfs where it's tough to scale to large numbers of files).

[jihoonson]

Storing audit logs in deep storage (like task reports) seems like a better idea to me than the metadata store. I suppose each one would need to be its own file. I would support moving them there, and maybe making them optional. I think they should be off by default -- they aren't too useful and they'll create a lot of extra files on deep storage (which people sometimes try to minimize, especially if they're on a system like hdfs where it's tough to scale to large numbers of files).

That sounds good to me. I'll update this PR soon. Thanks @jon-wei and @gianm.

[jihoonson]

Hmm, I've been thinking about this and would like to revert all changes in this PR and reduce this PR's scope to disable audit logging by default. The reasons are

• Moving audit logs to deep storage requires quite a lot of changes and introduces new design issues like audit log formats (single JSON per file, all JSONs in a single file, etc), log file name convention, how to efficiently search inserted/deleted segments from logs, how to manage increasing logs (like automatic cleanup) and so on. I don't want to make an effort for this kind of issue that there may not be a good use case.
• If there is any use case for http://<OVERLORD_IP>:<port>/druid/indexer/v1/task/{taskId}/segments, moving audit logs to deep storage will cause performance regression issue.
  • At first, I tried to address this issue by using a combination of inserted/deleted segment IDs as the file name, but it's not feasible because 1) there can be a lot of inserted/deleted segments and 2) some task actions requiring auditing (like ResetDataSourceMetadataAction) doesn't have segment information.
• If there is any user application based on taskLog table, there is still a workaround of increasing max size of a blob.

@gianm @jon-wei what do you think?

[jon-wei]

@jihoonson That sounds reasonable.

I agree that it's not worth the effort to migrate the whole audit log system to use deep storage given the various design considerations you mentioned, and the workaround is not too complex or burdensome.

[jihoonson]

Closing this in favor of #5859.