Skip to content

Add tableProperties to TableAuditEvent#601

Open
james5418 wants to merge 1 commit into
linkedin:mainfrom
james5418:jamewang/table-properties-audit-event
Open

Add tableProperties to TableAuditEvent#601
james5418 wants to merge 1 commit into
linkedin:mainfrom
james5418:jamewang/table-properties-audit-event

Conversation

@james5418
Copy link
Copy Markdown

Summary

Surface a snapshot of Iceberg table properties on the per-commit TableAuditEvent.

Changes

  • Client-facing API Changes
  • Internal API Changes
  • Bug Fixes
  • New Features
  • Performance Improvements
  • Code Style
  • Refactoring
  • Documentation
  • Tests

Testing Done

  • Manually Tested on local docker setup. Please include commands ran, and their output.
  • Added new tests for the changes made.
  • Updated existing tests to reflect the changes made.
  • No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
  • Some other form of testing like staging or soak time in production. Please explain.

./gradlew :services:tables:test --tests IcebergSnapshotsApiHandlerAuditTest

Additional Information

  • Breaking Changes
  • Deprecations
  • Large PR broken into smaller PRs, and PR plan linked in the description.

For all the boxes checked, include additional details of the changes made in this pull request.

.operationType(operationType);
.operationType(operationType)
.tableProperties(
createUpdateTableRequestBody == null
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tableProperties is read from createUpdateTableRequestBody (the client's request payload), not from the actual committed Iceberg table state. If OpenHouse adds or mutates properties server-side during commit (e.g. openhouse.tableVersion, openhouse.lastModifiedTime), those won't appear in the audit event.

The fix can be to get it from the response body. this response from claude looks like good next steps.

The fix is straightforward given what the aspect already has. result (the ApiResponse) is available after point.proceed(), and
result.getResponseBody() already has getTableLocation() called on it — getTableProperties() is right there too.
The change in TableAuditAspect.java: remove tableProperties from the pre-proceed() builder, delay .build() until after proceed() so you can read from the
response:

  // Remove the createUpdateTableRequestBody variable entirely.
  // Remove .tableProperties(...) from the builder chain.
  // Change the try block to:
  try {
    result = (ApiResponse<GetTableResponseBody>) point.proceed();
    TableAuditEvent event = eventBuilder
        .tableProperties(result.getResponseBody().getTableProperties())
        .build();
    buildAndSendEvent(event, OperationStatus.SUCCESS, result.getResponseBody().getTableLocation());
  } catch (Throwable t) {
    buildAndSendEvent(eventBuilder.build(), OperationStatus.FAILED, null);
    throw t;
  }

On the failure path eventBuilder.build() has no tableProperties — which is correct, there's no committed state to read from.
One thing to verify before doing this: confirm GetTableResponseBody has getTableProperties(). It almost certainly does since the response describes full
table state, but worth a quick grep in the codebase.

Copy link
Copy Markdown
Collaborator

@jiang95-dev jiang95-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case for adding table properties?

We should cap on the size of table properties as some can be quite large. See this PR: https://github.com/linkedin-multiproduct/li-openhouse/pull/688. Large events can cause OOM and losing events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants