Skip to content

Notifies systemSecurityMetadata on column add, rename, and drop events if using system security manager#16833

Merged
losipiuk merged 1 commit intotrinodb:masterfrom
codyzwief:codyzwief/system-metadata-column-events
Apr 13, 2023
Merged

Notifies systemSecurityMetadata on column add, rename, and drop events if using system security manager#16833
losipiuk merged 1 commit intotrinodb:masterfrom
codyzwief:codyzwief/system-metadata-column-events

Conversation

@codyzwief
Copy link
Copy Markdown
Contributor

@codyzwief codyzwief commented Mar 31, 2023

Description

This PR adds support for notifying SystemSecurityMetadata for column add/drop/rename events.

This was previously unsuccessfully implemented in this PR, but this PR bypasses the need to call getTableMetadata(session, tableHandle) by passing the tableName directly from the [Add|Rename|Drop]ColumnTask to get it to work with various connectors that don't allow it.

There will likely need to be coordination with this outstanding PR as the interface changes in this one will break the new tests in the linked one. Happy to wait to merge this change until #16759 has been merged, but looking for some feedback.

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Mar 31, 2023
@codyzwief codyzwief requested review from djsagain, ebyhr and findepi April 2, 2023 18:01
@codyzwief codyzwief marked this pull request as ready for review April 2, 2023 18:02
@codyzwief codyzwief force-pushed the codyzwief/system-metadata-column-events branch from 3fa59f1 to 99e3e9f Compare April 3, 2023 17:47
@codyzwief
Copy link
Copy Markdown
Contributor Author

@ebyhr I have made the requested changes. A couple of the tests are failing. I'd like to re-run the failing tests to see if they're consistently failing or if they're flaky tests, but I don't have permission to do so. Would it be possible to get those tests kicked off again?

Thanks for the quick review!

@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Apr 5, 2023

You can create an empty commit (git commit -am empty --allow-empty) and push it to trigger CI.

@codyzwief
Copy link
Copy Markdown
Contributor Author

Thanks @ebyhr! I have done that, and the tests have passed. Would it be possible to check again when you get a free moment?

Copy link
Copy Markdown
Member

@djsagain djsagain left a comment

Choose a reason for hiding this comment

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

This looked great! I left one hyper-nit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hyper-nit - - since it's only used only in the if stm, I would have been tempted to move the catalogMetadata value into the if stm here and in the other two methods.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought about that too. I'd rather keep it as-is though, personally; it matches the style of other methods that notify system security metadata.

@ebyhr ebyhr force-pushed the codyzwief/system-metadata-column-events branch from 092e4e3 to e85e94b Compare April 6, 2023 22:27
@ebyhr ebyhr force-pushed the codyzwief/system-metadata-column-events branch from e85e94b to 8020fda Compare April 6, 2023 22:28
@losipiuk losipiuk merged commit b887d98 into trinodb:master Apr 13, 2023
@github-actions github-actions bot added this to the 414 milestone Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants