Skip to content

Conversation

@kevinrr888
Copy link
Member

Prevents tableOperations().deleteRows of system tables in 4.0.

2.1 system tables:

  • root <-- cannot delete rows
  • metadata <-- cannot delete rows
  • replication <-- can deleteRows

4.0 system tables:

  • root <-- before: cannot delete rows, after: same
  • metadata <-- before: cannot delete rows, after: same
  • scan_ref <-- before: can delete rows, after: cannot delete rows
  • fate <-- before: can delete rows, after: cannot delete rows

I did not target 2.1 because I was unsure if we want deleteRows for the replication table (seems like that was the intent, but table expectations changed in 4.0).

Created from discussion: #5474 (comment)

@kevinrr888 kevinrr888 added this to the 4.0.0 milestone Apr 21, 2025
@kevinrr888 kevinrr888 self-assigned this Apr 21, 2025
@dlmarion
Copy link
Contributor

I can see why we wouldn't want users to delete from the fate table. It's managed by the Manager, which is supposed to always be running. However, I'm wondering if we want to allow delete rows on the scanref table. The entries are created by the ScanServers, and I don't remember how they are cleaned up. I'm wondering if there is a case where all the ScanServers are stopped, but entries still remain in the table. Probably worth looking at this aspect to determine if deleteRows is needed for the scanRef table.

@kevinrr888
Copy link
Member Author

deleteRows is never used internally for scanref, and I don't think we'd want a user to do any cleanup manually.
https://github.com/apache/accumulo/blob/main/server/base/src/main/java/org/apache/accumulo/server/metadata/ScanServerRefStoreImpl.java
Is how cleanup is handled. A BatchWriter is used to delete entries, so I think it's safe to disallow deleteRows for scanref

Copy link
Contributor

@keith-turner keith-turner left a comment

Choose a reason for hiding this comment

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

#5747 will eventually completely test these changes? Seems like thats the case, just wanted to make sure.

@kevinrr888
Copy link
Member Author

@keith-turner - Did you mean #5474? If so, yes that PR will test these changes

@kevinrr888 kevinrr888 merged commit 16e8043 into apache:main Apr 23, 2025
8 checks passed
@kevinrr888 kevinrr888 deleted the PreventDeleteRowsSysTables branch April 23, 2025 14:10
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.

4 participants