You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/04/27 22:56:22 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3350: modifies bulk import to use conditional mutations
keith-turner opened a new pull request, #3350:
URL: https://github.com/apache/accumulo/pull/3350
**Note** : this PR build on changes from #3317 which is not merged and that is why this PR is a draft. After #3317 is merged this will be rebased and taken out of draft.
Modifies the core of bulk import to use conditional mutations for
metadata table updates. Bulk import no longer request that the tserver
update the metadata table on behalf of the FATE transaction. If a tablet
is hosted the bulk import fate transaction sets a refresh column in the
metadata table and then ask the tserver to refresh its metadata. When
the tablet server does update its metadata it will delete the refresh
column.
This modification is incomplete. There is code that can be removed after
this changes. Also the refresh changes in the tablet servers are a hack
and have race conditions. Leaving the tserver changes half done was
intentional, that will be properly done in its own larger change to the
tserver. Issues will be opened for follow in changes.
The most important updates in this commit are in
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/*
along with a documention update in MetadataSchema.TabletsSection.RefreshIdColumnFamily
core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java
this give a good overview of the new refresh column
Fixes #3337
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#discussion_r1186380296
##########
test/src/main/java/org/apache/accumulo/test/functional/BulkSplitOptimizationIT.java:
##########
@@ -105,6 +107,8 @@ public void testBulkSplitOptimization() throws Exception {
// initiate splits
c.tableOperations().setProperty(tableName, Property.TABLE_SPLIT_THRESHOLD.getKey(), "100K");
+ c.tableOperations().setTabletHostingGoal(tableName, new Range(), TabletHostingGoal.ALWAYS);
Review Comment:
I added that so I could run the test and see it pass. It should eventually work w/o that. I'll remove it now because we will probably forget about it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1530335966
I think this approach coupled with a periodic check in the tserver might cover all bases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner merged pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3350:
URL: https://github.com/apache/accumulo/pull/3350
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1529986095
I wonder if the tablet refresh mechanism should be pulled out into a separate PR as a solution for #3310.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1529995969
I'm curious if you need a refresh column in the metadata. You could send the RPC to the hosting tablet server as a FaTE transaction and expect a response. If the call times out or fails, then you could retry as part of the FaTE framework.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#discussion_r1186385686
##########
test/src/main/java/org/apache/accumulo/test/functional/BulkNewIT.java:
##########
@@ -180,6 +181,8 @@ public void testSingleTabletSingleFile() throws Exception {
}
@Test
+ @Disabled("Need to implement set time functionality")
Review Comment:
I just added a comment about this test #3354.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3350: modifies bulk import to use conditional mutations
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#discussion_r1188839719
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -498,15 +497,4 @@ default void addBulkLoadInProgressFlag(String path, long fateTxid) {
default void removeBulkLoadInProgressFlag(String path) {
throw new UnsupportedOperationException();
}
-
- /**
- * Remove all the Bulk Load transaction ids from a given table's metadata
- *
- * @param tableId Table ID for transaction removals
- * @param tid Transaction ID to remove
- */
- default void removeBulkLoadEntries(TableId tableId, long tid) {
Review Comment:
This is going to conflict with #3336 . You might want to wait to merge this until #3336 is merged into 2.1 and up to elasticity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#discussion_r1189104188
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -498,15 +497,4 @@ default void addBulkLoadInProgressFlag(String path, long fateTxid) {
default void removeBulkLoadInProgressFlag(String path) {
throw new UnsupportedOperationException();
}
-
- /**
- * Remove all the Bulk Load transaction ids from a given table's metadata
- *
- * @param tableId Table ID for transaction removals
- * @param tid Transaction ID to remove
- */
- default void removeBulkLoadEntries(TableId tableId, long tid) {
Review Comment:
I merged #3336 into 2.1, main, elasticity and then this branch. Lots of conflicts to resolve along the way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1530121983
> It appears that a [check](https://github.com/apache/accumulo/blob/elasticity/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java#L779) for tablet metadata being out of date was already added (maybe in 2.1). I wonder if there is a way to consolidate some of these things on the tserver side.
We may be able to remove that because it based on different assumptions. It assumes the tablet reads the metadata and then only writes to it and is doing periodic validation. We are moving away from what is in memory on the tserver being the authority to whats in the metadata table being the authority.
> I wonder if it would just be easier to have a scan iterator on the tablet metadata that computes a a single hash value for the tablet metadata in the metadata table. The hash could be stored in memory in the TabletMetadata object in the TabletServer, and if it's different, then refresh.
That could avoid some uneeded refreshes, the tablet would need to track the last few hashes.
1. bulk import fate operation reads hash A
2. a compaction operation adds a file changing the hash to B
3. the compaction ask the tserver to refresh getting the file updates from the compaction and bulk import and it remembers the current hash of the files is B and adds A to the set that tracks the last N hashes.
4. the refresh request from bulk import arrives with hash A. The tserver ignores this because A is one of its previous hashes.
We could also have a file update counter in the tablet. Every time the set of files is updated in the tablet this counter is incremented by one. This increment can be safely done with conditional mutations, its like an atomic long in java. Refresh request just specify the counter they observed asking the tablet to update if its current counter is less than the one from the refresh RPC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1530098847
It appears that a [check](https://github.com/apache/accumulo/blob/elasticity/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java#L779) for tablet metadata being out of date was already added (maybe in 2.1). I wonder if there is a way to consolidate some of these things on the tserver side. Given that the TabletServer is going to have to periodically check the TabletMetadata to perform a refresh, I wonder if it would just be easier to have a scan iterator on the tablet metadata that computes a a single hash value for the tablet metadata in the metadata table. The hash could be stored in memory in the TabletMetadata object in the TabletServer, and if it's different, then refresh.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1530126605
I am going to try only having a refresh RPC and no refresh column to see what it looks like and see if there are any surprises.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1532280984
I rebased this after merging in #3317. I also added commit 9318029 which dropped the refresh column and started using a synchronous rpc for asking tserver to refresh tablet metadata. Took the PR out of draft after those changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1530052719
> I'm curious if you need a refresh column in the metadata. You could send the RPC to the hosting tablet server as a FaTE transaction and expect a response. If the call times out or fails, then you could retry as part of the FaTE framework.
I think that could be done and it may be more efficient for the normal case when everything is stable and working well. The refresh column handles the following cases that would not handled by an RPC alone.
* The refresh column is set with the location of the tserver at the time the files were added. So it can detect if the tablet has moved since it added the files and if it did move then it knows that no refresh is needed.
* In the case where the manager dies, the refresh column keeps track of what work is left across manager restarts.
* The transaction id in the refresh column allows the tablet server to know if it has already recently loaded something. For example if 3 fate transactions all want to refresh a tablet at around the same time the tablet can do one actual refresh and satisfy all request. Without the refresh column the tablet will always have reload its metadata when requested even if its not needed (because a recent reload it did satisfies the request).
* The refresh column allows FATE to process the refresh operations in the isReady call using async RPC which means a fate thread is not tied up. Without the refresh column a FATE thread would have to be tied up until all tablets are refreshed because the tracking of what is refreshed is moved from metadata table to memory. Also the Fate operation would have to use synchronous RPCs vs async. Also the fate thread has to keep everything in memory.
Using only an RPC, I think we could do the following.
* Scan the metadata table for tablets with load markers and build a map of `Map<TServerLoc, List<KeyExtent>>`. The map may contain some tservers that do not actually need to refresh because they were not the location when the files were set, but that does not hurt anythnig.
* Continue sending synchronous RPCs unil a positive response has been received for everything in the map.
Barring all the edge cases, this may be faster. The fate thread would be tied up for however long this takes. If all the fate threads are tied up, more can be added as long as memory supports it. There are other fate operations that hold a bunch of stuff in memory and/or tie up a fate thread for an indefinite time period while waiting for a condition to become true.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#discussion_r1186382935
##########
test/src/main/java/org/apache/accumulo/test/functional/BulkSplitOptimizationIT.java:
##########
@@ -105,6 +107,8 @@ public void testBulkSplitOptimization() throws Exception {
// initiate splits
c.tableOperations().setProperty(tableName, Property.TABLE_SPLIT_THRESHOLD.getKey(), "100K");
+ c.tableOperations().setTabletHostingGoal(tableName, new Range(), TabletHostingGoal.ALWAYS);
Review Comment:
removed in [27fc328](https://github.com/apache/accumulo/pull/3350/commits/27fc328d10979710023da6207f9eba92250dca3b)
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -119,6 +119,7 @@ public class TabletMetadata {
private TabletHostingGoal goal = TabletHostingGoal.ONDEMAND;
private boolean onDemandHostingRequested = false;
private TabletOperationId operationId;
+ private Map<Long,TServerInstance> refreshIds;
Review Comment:
no, removed in [27fc328](https://github.com/apache/accumulo/pull/3350/commits/27fc328d10979710023da6207f9eba92250dca3b)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1530329288
Experimented with changing just the RefreshTablets Repo for the FATE operation to see what it would look like if we dropped the refresh column and only sent RPCs.
```java
public class RefreshTablets extends ManagerRepo {
public long isReady(long tid, Manager manager) throws Exception {
// nothing to do here, always ready to run
return 0;
}
@Override
public Repo<Manager> call(long tid, Manager manager) throws Exception {
Map<Location,List<KeyExtent>> refreshesNeeded;
try (var tablets =
manager.getContext().getAmple().readTablets().forTable(bulkInfo.tableId).checkConsistency()
.fetch(ColumnType.LOADED, ColumnType.LOCATION, ColumnType.PREV_ROW).build()) {
// Find all tablets that have a location and load markers for this bulk load operation and
// therefore need to refresh their metadata. There may be some tablets in the map that were
// hosted after the bulk files were set and that is ok, it just results in an unneeded refresh
// request. There may also be tablets that had a location when the files were set but do not
// have a location now, that is ok the next time that tablet loads somewhere it will see the
// files.
refreshesNeeded = tablets.stream()
.filter(tabletMetadata -> tabletMetadata.getLocation() != null
&& tabletMetadata.getLoaded().containsValue(tid))
.collect(groupingBy(TabletMetadata::getLocation,
mapping(TabletMetadata::getExtent, toList())));
}
HashSet<Location> completed = new HashSet<>();
while (!refreshesNeeded.isEmpty()) {
for (Map.Entry<Location,List<KeyExtent>> entry : refreshesNeeded.entrySet()) {
try {
// TODO use a thread pool to send RPC request
// Ask tablet server to reload the metadata for these tablets. If the tabletserver is no
// longer serving a tablet that is ok as long is reloads the ones that it is
// still serving. If a tablet was unloaded after the map was created above when it
// reloads elsewhere it will see the bulk files. The tserver will need to ensure tablets
// that had future location and are in the process of loading are handled correctly or
// the refresh request could be missed due to race conditions in the tablet server.
sendSyncRefreshRequest(entry.getKey(), entry.getValue());
// refresh request returned successfully, so mark the location as completed
completed.add(entry.getKey());
} catch (Exception e) { // TODO what exceptions are ok to catch and retry on? this
// exception is too broad
if (tserverIsDead(entry.getKey())) {
// if the tserver is no longer alive then when tablets load elsewhere they will see the files, so mark it completed
completed.add(entry.getKey());
} else {
// TODO log something about waiting on this tserver for bulk import refresh request
}
}
}
// remove all tablet server that were successfully refreshed
refreshesNeeded.keySet().removeAll(completed);
if (!refreshesNeeded.isEmpty()) {
// TODO sleep and log using a retry object
}
}
return new CleanUpBulkImport(bulkInfo);
}
}
```
Learned a few things from this. Will probably need a thread pool and will need to handle tservers that die after getting the information from the metadata table. I also realized there could be some tricky issues with future locations with this approach, but those should be ok also long as the tserver handles concurrent tablet loads and refresh request correctly. I can't think of any cases with this approach where a tablet that needs to refresh does not.
Thinking of switching to this approach. It has pluses and minuses compared to the refresh column. Thinking its simpler to start with, likely faster, and if any of the problems the refresh column addresses show up then we can reconsider it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3350: modifies bulk import to use conditional mutations
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#issuecomment-1530342781
> I think this approach coupled with a periodic check in the tserver might cover all bases.
The period validation the tserver does to check memory vs metadata can be changed to just refresh metadata (and drop the validation against whats in memory aspect of what it does).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3350: modifies bulk import to use conditional mutations
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3350:
URL: https://github.com/apache/accumulo/pull/3350#discussion_r1186253911
##########
test/src/main/java/org/apache/accumulo/test/functional/BulkNewIT.java:
##########
@@ -180,6 +181,8 @@ public void testSingleTabletSingleFile() throws Exception {
}
@Test
+ @Disabled("Need to implement set time functionality")
Review Comment:
I think we should leave the test broken. I saw the follow-on issue that you created to fix this.
##########
test/src/main/java/org/apache/accumulo/test/functional/BulkSplitOptimizationIT.java:
##########
@@ -105,6 +107,8 @@ public void testBulkSplitOptimization() throws Exception {
// initiate splits
c.tableOperations().setProperty(tableName, Property.TABLE_SPLIT_THRESHOLD.getKey(), "100K");
+ c.tableOperations().setTabletHostingGoal(tableName, new Range(), TabletHostingGoal.ALWAYS);
Review Comment:
Are you adding this so that the test passes? If so, should we leave the test broken so that we can fix it when we move the split logic to the Manager for hosted and unhosted tablets?
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -119,6 +119,7 @@ public class TabletMetadata {
private TabletHostingGoal goal = TabletHostingGoal.ONDEMAND;
private boolean onDemandHostingRequested = false;
private TabletOperationId operationId;
+ private Map<Long,TServerInstance> refreshIds;
Review Comment:
is this still being used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org