-
Notifications
You must be signed in to change notification settings - Fork 284
Replace ClientContext/TabletLocator/MetadataServicer with AccumuloTableInfoFetcher facade #3449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: integration
Are you sure you want to change the base?
Changes from 2 commits
1c86b79
5f29ebf
0172f51
2c33a78
a488b60
0c04d33
44e5aa0
be1a2e1
31bb688
4ffad05
619d710
0e166e9
516f3c6
d2c0891
40f003f
19772a9
60f25db
87c1cfe
866f286
3089e5e
1c463ca
7428623
b243082
54cb9d9
785f751
e20b985
64e8cbc
50b27a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| package datawave.core.common.connection; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.List; | ||
|
|
||
| import org.apache.accumulo.core.client.AccumuloClient; | ||
| import org.apache.accumulo.core.client.AccumuloException; | ||
| import org.apache.accumulo.core.client.AccumuloSecurityException; | ||
| import org.apache.accumulo.core.client.TableNotFoundException; | ||
| import org.apache.accumulo.core.client.admin.ActiveCompaction; | ||
| import org.apache.accumulo.core.client.admin.Locations; | ||
| import org.apache.accumulo.core.data.Range; | ||
| import org.apache.accumulo.core.data.TableId; | ||
|
|
||
| /** | ||
| * Facade that centralizes Accumulo table metadata operations behind public APIs. | ||
| * <p> | ||
| * This class replaces direct usage of non-public Accumulo internals ({@code ClientContext}, {@code ThriftClientTypes}, {@code TabletLocator}, | ||
| * {@code MetadataServicer}, etc.) with their public API equivalents. All methods delegate to {@link org.apache.accumulo.core.client.admin.TableOperations} or | ||
| * {@link org.apache.accumulo.core.client.admin.InstanceOperations}. | ||
| * | ||
| * @see <a href="https://github.com/NationalSecurityAgency/datawave/issues/2443">Issue #2443</a> | ||
| */ | ||
| public class AccumuloTableInfoFetcher { | ||
|
|
||
| private final AccumuloClient client; | ||
|
|
||
| public AccumuloTableInfoFetcher(AccumuloClient client) { | ||
| this.client = client; | ||
| } | ||
|
|
||
| /** | ||
| * Get the TableId for a table name using the public {@code tableIdMap()} API. | ||
|
SethSmucker marked this conversation as resolved.
Outdated
|
||
| * | ||
| * @param tableName | ||
| * the table name | ||
| * @return the TableId | ||
| * @throws TableNotFoundException | ||
| * if the table does not exist | ||
| */ | ||
| public TableId getTableId(String tableName) throws TableNotFoundException { | ||
| String id = client.tableOperations().tableIdMap().get(tableName); | ||
| if (id == null) { | ||
| throw new TableNotFoundException(null, tableName, "Table not found in tableIdMap"); | ||
| } | ||
| return TableId.of(id); | ||
| } | ||
|
|
||
| /** | ||
| * Check if a table exists using the public {@code exists()} API. | ||
| * | ||
| * @param tableName | ||
| * the table name | ||
| * @return true if the table exists | ||
| */ | ||
| public boolean tableExists(String tableName) { | ||
| return client.tableOperations().exists(tableName); | ||
| } | ||
|
|
||
| /** | ||
| * Check if a table is online using the public {@code isOnline()} API. | ||
| * | ||
| * @param tableName | ||
| * the table name | ||
| * @return true if the table is online | ||
| * @throws TableNotFoundException | ||
| * if the table does not exist | ||
| * @throws AccumuloException | ||
| * if a general Accumulo error occurs | ||
| */ | ||
| public boolean isTableOnline(String tableName) throws TableNotFoundException, AccumuloException { | ||
| return client.tableOperations().isOnline(tableName); | ||
| } | ||
|
|
||
| /** | ||
| * Get tablet locations for the given ranges using the public {@code locate()} API. | ||
| * | ||
| * @param tableName | ||
| * the table name | ||
| * @param ranges | ||
| * the ranges to locate | ||
| * @return the Locations result | ||
| * @throws TableNotFoundException | ||
| * if the table does not exist | ||
| * @throws AccumuloException | ||
| * if a general Accumulo error occurs | ||
| * @throws AccumuloSecurityException | ||
| * if a security error occurs | ||
| */ | ||
| public Locations getTabletLocations(String tableName, Collection<Range> ranges) | ||
| throws TableNotFoundException, AccumuloException, AccumuloSecurityException { | ||
| return client.tableOperations().locate(tableName, ranges); | ||
| } | ||
|
|
||
| /** | ||
| * Get the count of running major compactions across all tablet servers using the public {@code getActiveCompactions()} API. | ||
| * <p> | ||
| * Note: This counts only running compactions (not queued), which differs slightly from the original Thrift-based implementation that also counted queued | ||
| * compactions. This is acceptable because the MAJC_THRESHOLD default is 3000 (a high safety margin) and this is polled on each bulk load cycle. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SethSmucker are there any available APIs to get queued compactions? If so, can we just add it to maintain the historical behavior?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I could find for 2.1.* at least, but it looks like it will be available once we move to 4.0. For now I'll use the same facade pattern and we can swap out the internals once we hit 4.0, removing the facades one by one so we can test them on the high side. I'll push a draft for that soon, but if there's another way to do it that would work better I'm all ears
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, it looked like MetadataServicer might not need a class replacement, the public API versions of its usecase are fairly straight forward, so I think we can directly call them instead of having the original facade. We'll still have the one I mentioned above for the compactions, but just not the original one from the ticket. If it would be better to keep it as a centralized place (closer to what MetadataServicer was doing) I can revert it, but I'll go ahead and try out the direct method calls for now. |
||
| * | ||
| * @return the number of active major compactions | ||
| * @throws AccumuloException | ||
| * if a general Accumulo error occurs | ||
| * @throws AccumuloSecurityException | ||
| * if a security error occurs | ||
| */ | ||
| public int getMajorCompactionCount() throws AccumuloException, AccumuloSecurityException { | ||
| int count = 0; | ||
| List<ActiveCompaction> compactions = client.instanceOperations().getActiveCompactions(); | ||
| for (ActiveCompaction compaction : compactions) { | ||
| if (compaction.getType() == ActiveCompaction.CompactionType.MAJOR || compaction.getType() == ActiveCompaction.CompactionType.FULL) { | ||
| count++; | ||
| } | ||
| } | ||
| return count; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| package datawave.core.common.connection; | ||
|
|
||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import org.apache.accumulo.core.client.AccumuloClient; | ||
| import org.apache.accumulo.core.client.TableNotFoundException; | ||
| import org.apache.accumulo.core.client.admin.Locations; | ||
| import org.apache.accumulo.core.data.Range; | ||
| import org.apache.accumulo.core.data.TableId; | ||
| import org.apache.accumulo.core.data.TabletId; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
|
|
||
| import datawave.accumulo.inmemory.InMemoryAccumuloClient; | ||
| import datawave.accumulo.inmemory.InMemoryInstance; | ||
|
|
||
| public class AccumuloTableInfoFetcherTest { | ||
|
|
||
| private static final String TEST_TABLE = "testTable"; | ||
| private AccumuloClient client; | ||
| private AccumuloTableInfoFetcher fetcher; | ||
|
|
||
| @Before | ||
| public void setup() throws Exception { | ||
| InMemoryInstance instance = new InMemoryInstance(); | ||
| client = new InMemoryAccumuloClient("root", instance); | ||
| client.tableOperations().create(TEST_TABLE); | ||
| fetcher = new AccumuloTableInfoFetcher(client); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetTableId() throws Exception { | ||
| TableId tableId = fetcher.getTableId(TEST_TABLE); | ||
| assertNotNull(tableId); | ||
| // Verify it matches the tableIdMap entry | ||
| String expectedId = client.tableOperations().tableIdMap().get(TEST_TABLE); | ||
| assertEquals(expectedId, tableId.canonical()); | ||
| } | ||
|
|
||
| @Test(expected = TableNotFoundException.class) | ||
| public void testGetTableIdNonExistent() throws Exception { | ||
| fetcher.getTableId("nonExistentTable"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testTableExists() { | ||
| assertTrue(fetcher.tableExists(TEST_TABLE)); | ||
| assertFalse(fetcher.tableExists("nonExistentTable")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testIsTableOnline() throws Exception { | ||
| // InMemoryAccumuloClient.isOnline() returns false, but the API call should succeed | ||
| boolean online = fetcher.isTableOnline(TEST_TABLE); | ||
| // InMemory always returns false; just verify no exception is thrown | ||
| assertFalse(online); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetTabletLocations() throws Exception { | ||
| Locations locations = fetcher.getTabletLocations(TEST_TABLE, Collections.singletonList(new Range())); | ||
| assertNotNull(locations); | ||
| Map<TabletId,List<Range>> byTablet = locations.groupByTablet(); | ||
| assertNotNull(byTablet); | ||
| assertFalse(byTablet.isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetMajorCompactionCount() throws Exception { | ||
| // InMemoryAccumuloClient returns empty list for getActiveCompactions | ||
| int count = fetcher.getMajorCompactionCount(); | ||
| assertEquals(0, count); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.