Comprehensive table operations IT #5474

kevinrr888 · 2025-04-14T15:56:40Z

This PR:

Creates a comprehensive IT of all table operations against both system and user tables while trying to avoid duplicating existing testing. For example, when there was more extensive testing of a table operation elsewhere, these new tests just ensure that that testing still exists. This IT does not test for edge cases, but rather tests for basic expected functionality of all table operations against user tables and all system tables (ROOT, METADATA, FATE, SCAN_REF).

Motivation:

No single test testing all table operations making it hard to know what is or isn't tested
Lots of operations that aren't tested on all system tables and some operations that are completely untested. Good to ensure each table op works as expected on the different system tables as well as user tables. For example, some ops can't be performed on the metadata tables (ROOT, METADATA) but can be performed on FATE and SCAN_REF, some ops aren't allowed on any system tables, etc.
Has made identifying potential issues related to what ops are allowed on what system tables easier. For example, have discovered that offlineing the FATE and SCAN_REF tables are allowed through the table operations API when they shouldn't be (prevent offline all system tables #5433)
Ensures end-to-end functionality of table operations always work as expected
Ensures FATE, as it's used in the Manager, is functioning as expected for both META (FATE ops against system tables) and USER (FATE ops against user tables) FATE ops. This is important since they use different resources and different stores. Lots of individual FATE tests, but this verifies FATE is working as expected in the Manager.
Will force any newly added table operations to have testing here.
Will force any new system tables to be considered in these tests.

closes #5427

Creates a comprehensive IT of all table operations against both system and user tables while trying to avoid duplicating existing testing. For example, when there was more extensive testing of a table operation elsewhere, these new tests just ensure that that testing still exists. This IT does not test for edge cases, but rather tests for basic expected functionality of all table operations against user tables and all system tables (ROOT, METADATA, FATE, SCAN_REF).

kevinrr888 · 2025-04-14T15:59:12Z

test/src/main/java/org/apache/accumulo/test/ComprehensiveTableOperationsIT.java

+      for (var sysTable : List.of(AccumuloTable.ROOT, AccumuloTable.METADATA,
+          AccumuloTable.SCAN_REF, AccumuloTable.FATE)) {


TLDR: Test does not work. Potential issue with compacting system tables but more likely issue with testing approach.

This test is not working as expected. Only passes when compacting the FATE table is done last. The root cause of the issue is the call to createFateTableRow(userTable). This initiates a very slow compaction on the userTable that will never complete until cancelled. This is done to create some data in the FATE table and flush the table before we compact. Compacting the FATE table with this compaction FATE op in the FATE table works as expected (compaction completes on FATE table as expected). However, compacting subsequent system tables do not work as expected. One way to see this is putting FATE before SCAN_REF in the list. You can see that the compaction on SCAN_REF hangs indefinitely. It gets stuck on the CompactionDriver.isReady() step. Considered that the slow compaction which would still be running on the userTable is somehow causing the issue, but even if the compaction is cancelled and we wait for it to be completely cancelled and cleaned up from the FATE table, the issue still occurs on subsequent system table compactions. Further, the compaction on the userTable should have no effect on compactions on the system tables since compacting the userTable won't lock the accumulo namespace or anything like that. I have not been able to figure out this issue, if someone can spot an issue with my testing approach or look at the Manager log during the described failure case and figure out what is wrong, that would be much appreciated.

When I run this test case on its own it passes. But does time out when I run all tests in this class.

Yeah, it will pass on its own but not when all tests are run because some other tests call createFateTableRow(userTable) which is the root cause of the problem.

I can take a look into this. If this is not figured out before this PR is merged then we should open an issue for this.

That would be great.

test/src/main/java/org/apache/accumulo/test/ComprehensiveTableOperationsIT.java

kevinrr888 · 2025-04-14T16:05:13Z

test/src/main/java/org/apache/accumulo/test/ComprehensiveTableOperationsIT.java

+      assertThrows(IllegalArgumentException.class, () -> ops
+          .setTabletAvailability(sysTable.tableName(), new Range(), TabletAvailability.UNHOSTED));


Should maybe be an AccumuloException to keep consistency with the other table operations methods that throw an AccumuloException when performed on an unacceptable table?

This unreleased code, so now is the time to change exceptions. Makes sense to me to change it to AccumuloException. One important thing is making sure the expected exceptions are noted in the javadoc, that helps developers and users know what is expected going forward.

Opened #5510

A lot of the table operations don't have javadoc for what the thrown exceptions mean (including setTabletAvailability in this case), so maybe this can be it's own ticket

test/src/main/java/org/apache/accumulo/test/ComprehensiveTableOperationsIT.java

core/src/main/java/org/apache/accumulo/core/clientImpl/ClientTabletCacheImpl.java

kevinrr888 · 2025-04-14T16:06:21Z

test/src/main/java/org/apache/accumulo/test/functional/CloneTestIT.java

+        assertThrows(Exception.class, () -> client.tableOperations().clone(sysTable.tableName(),
+            cloneTableName, CloneConfiguration.empty()));


Same comment about some throwing AccumuloException and some throwing AccumuloSecurityException

keith-turner

Still looking at this. Posting comments I have so far.

test/src/main/java/org/apache/accumulo/test/ComprehensiveTableOperationsIT.java

- Updated tests based on recent changes in main to `offline` and `getDiskUsage` - Address review comments - Trivial misc cleanup

test/src/main/java/org/apache/accumulo/test/ComprehensiveTableOperationsIT.java

DomGarguilo · 2025-04-18T19:47:10Z

test/src/main/java/org/apache/accumulo/test/ComprehensiveTableOperationsIT.java

+      for (var sysTable : List.of(AccumuloTable.ROOT, AccumuloTable.METADATA,
+          AccumuloTable.SCAN_REF, AccumuloTable.FATE)) {


When I run this test case on its own it passes. But does time out when I run all tests in this class.

test/src/main/java/org/apache/accumulo/test/ComprehensiveTableOperationsIT.java

- Compare sets of files instead of number of files in flush - Extra check to test_setProperty_modifyProperties_removeProperty_getProperties_getTableProperties_getConfiguration

Making use of new helper methods in SystemTables

kevinrr888 self-assigned this Apr 14, 2025

kevinrr888 added this to the 4.0.0 milestone Apr 14, 2025

kevinrr888 commented Apr 14, 2025

View reviewed changes

Merge branch 'main' into 4.0-feature-5427

eb2a46e

This was referenced Apr 15, 2025

Fixes bug with getting disk usage of the ROOT table #5479

Merged

prevent offline all system tables #5433

Merged

DomGarguilo self-requested a review April 16, 2025 21:07

keith-turner reviewed Apr 16, 2025

View reviewed changes

Merge branch 'main' into 4.0-feature-5427

42c15ea

kevinrr888 mentioned this pull request Apr 18, 2025

adds early return for all sys tables in ClientTabletCacheImpl.request… #5487

Merged

Changes:

4eb99eb

- Updated tests based on recent changes in main to `offline` and `getDiskUsage` - Address review comments - Trivial misc cleanup

DomGarguilo approved these changes Apr 18, 2025

View reviewed changes

kevinrr888 added 2 commits April 18, 2025 16:46

address review

41599c1

Merge branch 'main' into 4.0-feature-5427

5270a11

kevinrr888 mentioned this pull request Apr 21, 2025

prevent deleteRows of system tables #5491

Merged

keith-turner reviewed Apr 22, 2025

View reviewed changes

keith-turner mentioned this pull request Apr 22, 2025

Modified IteratorEnvironment to no longer throw UOE #5490

Merged

kevinrr888 added 2 commits April 25, 2025 11:25

Merge branch 'main' into 4.0-feature-5427

8a51b2f

Address review:

c882631

- Compare sets of files instead of number of files in flush - Extra check to test_setProperty_modifyProperties_removeProperty_getProperties_getTableProperties_getConfiguration

kevinrr888 mentioned this pull request Apr 25, 2025

Minor TableOperations changes #5510

Merged

DomGarguilo and others added 4 commits April 28, 2025 16:19

Making use of new helper methods in SystemTables

0badef9

Merge pull request #2 from DomGarguilo/kevinBranchChanges

53a4a3e

Making use of new helper methods in SystemTables

improved test_rename

d49df4e

formatting

53ba7f2

		for (var sysTable : List.of(AccumuloTable.ROOT, AccumuloTable.METADATA,
		AccumuloTable.SCAN_REF, AccumuloTable.FATE)) {

		assertThrows(IllegalArgumentException.class, () -> ops
		.setTabletAvailability(sysTable.tableName(), new Range(), TabletAvailability.UNHOSTED));

		assertThrows(Exception.class, () -> client.tableOperations().clone(sysTable.tableName(),
		cloneTableName, CloneConfiguration.empty()));

Comprehensive table operations IT #5474

Are you sure you want to change the base?

Comprehensive table operations IT #5474

Uh oh!

Conversation

kevinrr888 commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrr888 Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinrr888 commented Apr 14, 2025 •

edited

Loading

kevinrr888 Apr 25, 2025 •

edited

Loading