You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2024/03/27 12:11:06 UTC
[PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
dlmarion opened a new pull request, #4421:
URL: https://github.com/apache/accumulo/pull/4421
Modified the tablet mutating implementations (TabletMutator, ConditionalTabletMutator, etc.) to always update the srv:lock column so that the MetadataConstraints filter on the server side validates that the update comes from a "valid" server. I had to modify MiniAccumuloClusterImpl to acquire a ServiceLock for the ITs to use where the test code is updating the tablet metadata. The changes in the other classes are mainly just making sure that the tablet mutator classes are getting the ServiceLock.
Closes #4420
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1557961458
##########
server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java:
##########
@@ -76,9 +77,9 @@ public ServerAmpleImpl(ServerContext context) {
}
@Override
- public Ample.TabletMutator mutateTablet(KeyExtent extent) {
+ public Ample.TabletMutator mutateTablet(KeyExtent extent, ServiceLock lock) {
Review Comment:
Yeah, I'm not opposed to changing the plumbing
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1554287799
##########
server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java:
##########
@@ -76,9 +77,9 @@ public ServerAmpleImpl(ServerContext context) {
}
@Override
- public Ample.TabletMutator mutateTablet(KeyExtent extent) {
+ public Ample.TabletMutator mutateTablet(KeyExtent extent, ServiceLock lock) {
Review Comment:
@cshannon's suggestion:
`Looking at this, is it possible to move the passing of the lock to the construction of the mutator?`
I'm fine with changing the how/where the lock is supplied. The biggest issue I'm having right now is I can't figure out why BigRootTabletIT is failing.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2051876083
@cshannon - Thanks for this information. I submitted #4453 that fixes BigRootTabletIT in elasticity. Seems like always adding `srv:lock` pushes the root tablet over the threshold in this branch. I think that it also means that the root tablet in the elasticity branch is less than `1024` bytes, meaning the test isn't really testing the condition.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1544583014
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java:
##########
@@ -558,6 +558,11 @@ public synchronized void unlock() throws InterruptedException, KeeperException {
LOG.debug("[{}] Deleting all at path {} due to unlock", vmLockPrefix, pathToDelete);
ZooUtil.recursiveDelete(zooKeeper, pathToDelete, NodeMissingPolicy.SKIP);
+ // Wait for the delete to happen on the server before exiting method
+ while (zooKeeper.exists(pathToDelete, null) != null) {
Review Comment:
Do we need a timeout here in case something goes wrong with the delete? We may not as any server issues get rethrown and should break but was something I was wondering, if there's a scenario we could get stuck in this loop forever.
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/AbstractTabletStateStore.java:
##########
@@ -38,16 +40,22 @@
public abstract class AbstractTabletStateStore implements TabletStateStore {
private final Ample ample;
+ private final Supplier<ServiceLock> lock;
- protected AbstractTabletStateStore(ClientContext context) {
+ protected AbstractTabletStateStore(ClientContext context, Supplier<ServiceLock> lock) {
this.ample = context.getAmple();
+ this.lock = lock;
+ }
+
+ protected ServiceLock getLock() {
+ return lock.get();
}
@Override
public void setLocations(Collection<Assignment> assignments) throws DistributedStoreException {
try (var tabletsMutator = ample.conditionallyMutateTablets()) {
for (Assignment assignment : assignments) {
- var conditionalMutator = tabletsMutator.mutateTablet(assignment.tablet)
+ var conditionalMutator = tabletsMutator.mutateTablet(assignment.tablet, lock.get())
Review Comment:
Looking at this, is it possible to move the passing of the lock to the construction of the mutator? It seems like it makes more sense there as the scope of the lock should be tied to the mutator instance life. This seems cleaner vs having to pass the lock to every mutateTablet() call.
For example:
```java
try (var tabletsMutator = ample.conditionallyMutateTablets(lock.get())) {
for (Assignment assignment : assignments) {
var conditionalMutator = tabletsMutator.mutateTablet(assignment.tablet)
.requireLocation(TabletMetadata.Location.future(assignment.server))
.putLocation(TabletMetadata.Location.current(assignment.server))
.deleteLocation(TabletMetadata.Location.future(assignment.server)).deleteSuspension();
...
});
}
```
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2051604841
> BigRootTabletIT is still failing. From what I can determine, the TabletManagementIterator is just returning metadata for the first tablet after the split over and over again until the test times out.
The test failure with `BigRootTabletIT` doesn't appear to be anything to do with this PR and I think should be opened up as a separate issue. There is a definitely a change of behavior here that needs to be looked, but it seems related more to the overall changes to Elasticity than anything here. With the current setting it's just getting stuck in a loop because it's reporting too high memory usage as I mentioned in [this](https://github.com/apache/accumulo/pull/4421#issuecomment-2027789505) comment. My assumption is it's simply because of the extra data now with the new column
For reference, I compared the modified test here against other branches and different settings to see when it would get stuck on the `waitForBalance()` call. The results were that Elasticity also just hangs in the same way if you drop memory a little bit but Main always seems to work. The test passes with this PR if you bump `TABLE_SCAN_MAXMEM` a little to 1152.
| Branch | Max Mem | Result |
| -------- | ------- | ------- |
| Main | 1024 | Pass |
| Main | 992| Pass |
| Main | 128| Pass |
| Elasticity | 992 | Fail |
| Elasticity | 1024 | Pass |
| PR 4421 | 1024 | Fail |
| PR 4421 | 1088 | Fail |
| PR 4421 | 1152 | Pass |
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2022664116
I don't know if there is a downside to *always* setting `srv:lock` when updating the metadata table. MetadataConstraints uses ZooCache when [checking](https://github.com/apache/accumulo/blob/elasticity/server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java#L382) the ServiceLock so the load on ZooKeeper, in theory, should not be increased. Given that a good portion of the updates are now coming from a Manager vs the TabletServer hosting the Tablet, and that some of these operations can be performed on unhosted tablets, the number of entries in ZooCache could be much less than in previous releases.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1557945817
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java:
##########
@@ -558,6 +558,11 @@ public synchronized void unlock() throws InterruptedException, KeeperException {
LOG.debug("[{}] Deleting all at path {} due to unlock", vmLockPrefix, pathToDelete);
ZooUtil.recursiveDelete(zooKeeper, pathToDelete, NodeMissingPolicy.SKIP);
+ // Wait for the delete to happen on the server before exiting method
+ while (zooKeeper.exists(pathToDelete, null) != null) {
Review Comment:
`ServiceLock.unlock`is only called from the server processes when they are shutting down and from the tests. Not waiting for the locks to be removed caused issues in tests where the cluster was restarted in between test methods. I think for the tests we need to wait, but maybe I'll add some logging here so that we know that the server is waiting to shutdown because the locks are not removed. The user can then have the option to forcefully terminate the process if they want.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "RCQE-bot (via GitHub)" <gi...@apache.org>.
RCQE-bot commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2058853576
## RCQE-bot: Commit Quality Assessment 🔎
I've carefully evaluated the recent commits, and here's the current state of the commit quality:
### Metrics 📊
- Previous Commits: diff hunk in file [server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java](https://github.com/apache/accumulo/pull/4421/files/a202f818f35af0bfb63bcdf327ef55f33acae798#diff-786b6c28ea242918ee67f01720a40235db69be8a4c35bc920dcc45e29fde6c3d) in commit [f3c1adebe64a5bf1689a5a1cc26e09be375c7b85](https://github.com/apache/accumulo/pull/4421/commits/f3c1adebe64a5bf1689a5a1cc26e09be375c7b85)
- Code Review: Not sure if this will be better. These changes require passing the service lock around all of the time. Was wondering if we could instead pass the service lock in when creating ServerAmpleImpl. Maybe this would not require passing the service lock to so many methods.
### Suggestions 💡
- Need Review Again 🚫 : The commits do not currently meet the quality standards. I recommend not merging them into the main branch until the above suggestions have been implemented and a subsequent review confirms the quality. Please address the points mentioned and submit the commits for another round of review.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1566341462
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/AbstractTabletStateStore.java:
##########
@@ -38,16 +40,22 @@
public abstract class AbstractTabletStateStore implements TabletStateStore {
private final Ample ample;
+ private final Supplier<ServiceLock> lock;
- protected AbstractTabletStateStore(ClientContext context) {
+ protected AbstractTabletStateStore(ClientContext context, Supplier<ServiceLock> lock) {
this.ample = context.getAmple();
+ this.lock = lock;
+ }
+
+ protected ServiceLock getLock() {
+ return lock.get();
}
@Override
public void setLocations(Collection<Assignment> assignments) throws DistributedStoreException {
try (var tabletsMutator = ample.conditionallyMutateTablets()) {
for (Assignment assignment : assignments) {
- var conditionalMutator = tabletsMutator.mutateTablet(assignment.tablet)
+ var conditionalMutator = tabletsMutator.mutateTablet(assignment.tablet, lock.get())
Review Comment:
I re-worked the service lock plumbing in 0243ff0.
##########
server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java:
##########
@@ -76,9 +77,9 @@ public ServerAmpleImpl(ServerContext context) {
}
@Override
- public Ample.TabletMutator mutateTablet(KeyExtent extent) {
+ public Ample.TabletMutator mutateTablet(KeyExtent extent, ServiceLock lock) {
Review Comment:
I re-worked the service lock plumbing in 0243ff0.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2027789505
> I think the only test failing at this point is BigRootTabletIT. I've spent probably 4 hours trying to track down the issue, and I'm unable to figure it out. After the split on the metadata table happens, the Manager doesn't appear to be hosting the new metadata tablets.
I looked at this for a while to and it seems to be related to the low memory check inside `TabletBase`. The `TABLE_SCAN_MAXMEM` config is used in a couple spots, but if I bump the `maxResultsSize` value when testing specifically just on [this](https://github.com/dlmarion/accumulo/blob/7e5c7ebc96300d4063b242f6fb7dbc0c02997ae1/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/TabletBase.java#L421) line the test works again. That maxResultsSize is controlled by that config property so I'm wondering if the extra data being written pushes it over the edge for being able to continue.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1554293257
##########
server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java:
##########
@@ -76,9 +77,9 @@ public ServerAmpleImpl(ServerContext context) {
}
@Override
- public Ample.TabletMutator mutateTablet(KeyExtent extent) {
+ public Ample.TabletMutator mutateTablet(KeyExtent extent, ServiceLock lock) {
Review Comment:
Re the supplier in my initial comment, I initially thought about having a setter method instead. Was thinking the supplier made apparent that its a mandatory requirement to provide it even though its not immediately available. A setter is something that someone could forget to call.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2058859737
What is this bot? I can't find anything about it via Google search.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "jamietanna (via GitHub)" <gi...@apache.org>.
jamietanna commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2058951785
We've also had it hit some of our (Elastic) repos - might be worth blocking from the Apache GitHub organisation
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2045989576
BigRootTabletIT is still failing. From what I can determine, the TabletManagementIterator is just returning metadata for the same tablet over and over again until the test times out.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2056675641
> I spent many hours looking at this.
Same, over several days. I'm glad you found and fixed it. I think the only remaining issue based on the comments in this PR is where/how the ServiceLock is supplied to Ample. I'm going to review the comments and address this last issue.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2039675893
BigRootTabletIT is still failing, but the build succeeds now
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2052649553
@keith-turner infinite loop is exactly what I was seeing. Thanks for tracking this down.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2059546599
> In latest IT build only the SplitMillionIT timed out, no other failures. SplitMillionIT timed out once for me locally, and succeeded twice. So I think it's just flaky. I have no other planned changes for this PR.
Yeah that test is flaky, it's set for a 10 minute timeout and when I've seen it pass it still takes like 9 minutes and 45 seconds. I think we need to increase the amount of time for that test.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #4421:
URL: https://github.com/apache/accumulo/pull/4421
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1544829442
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/AbstractTabletStateStore.java:
##########
@@ -38,16 +40,22 @@
public abstract class AbstractTabletStateStore implements TabletStateStore {
private final Ample ample;
+ private final Supplier<ServiceLock> lock;
- protected AbstractTabletStateStore(ClientContext context) {
+ protected AbstractTabletStateStore(ClientContext context, Supplier<ServiceLock> lock) {
this.ample = context.getAmple();
+ this.lock = lock;
+ }
+
+ protected ServiceLock getLock() {
+ return lock.get();
}
@Override
public void setLocations(Collection<Assignment> assignments) throws DistributedStoreException {
try (var tabletsMutator = ample.conditionallyMutateTablets()) {
for (Assignment assignment : assignments) {
- var conditionalMutator = tabletsMutator.mutateTablet(assignment.tablet)
+ var conditionalMutator = tabletsMutator.mutateTablet(assignment.tablet, lock.get())
Review Comment:
> Looking at this, is it possible to move the passing of the lock to the construction of the mutator?
I have no issue with that. I can make that change after all the tests are passing.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1554284423
##########
server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java:
##########
@@ -76,9 +77,9 @@ public ServerAmpleImpl(ServerContext context) {
}
@Override
- public Ample.TabletMutator mutateTablet(KeyExtent extent) {
+ public Ample.TabletMutator mutateTablet(KeyExtent extent, ServiceLock lock) {
Review Comment:
Not sure if this will be better. These changes require passing the service lock around all of the time. Was wondering if we could instead pass the service lock in when creating ServerAmpleImpl. Maybe this would not require passing the service lock to so many methods.
Was playing around w/ the idea the when the ServerContext is created a supplier is passed in the for the service lock. The idea being that server processes could set it later when its available. Something like AtomicReference.get could be used as the impl for the supplier. The tserver creates the AtomicRef and passes it to the ServerContext and later when it creates the ServiceLock it sets the atomic ref.
```diff
diff --git a/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java b/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
index e58645d00b..7ca94fd785 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
@@ -103,12 +103,13 @@ public class ServerContext extends ClientContext {
private final Supplier<AuditedSecurityOperation> securityOperation;
private final Supplier<CryptoServiceFactory> cryptoFactorySupplier;
private final Supplier<LowMemoryDetector> lowMemoryDetector;
+ private final Supplier<ServiceLock> serviceLockSupplier;
- public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ public ServerContext(SiteConfiguration siteConfig, Supplier<ServiceLock> serviceLockSupplier) {
+ this(new ServerInfo(siteConfig), serviceLockSupplier);
}
- private ServerContext(ServerInfo info) {
+ private ServerContext(ServerInfo info, Supplier<ServiceLock> serviceLockSupplier) {
super(SingletonReservation.noop(), info, info.getSiteConfiguration(), Threads.UEH);
this.info = info;
zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
@@ -129,6 +130,7 @@ public class ServerContext extends ClientContext {
memoize(() -> new AuditedSecurityOperation(this, SecurityOperation.getAuthorizor(this),
SecurityOperation.getAuthenticator(this), SecurityOperation.getPermHandler(this)));
lowMemoryDetector = memoize(() -> new LowMemoryDetector());
+ this.serviceLockSupplier = serviceLockSupplier;
}
/**
@@ -275,8 +277,13 @@ public class ServerContext extends ClientContext {
return cryptoFactorySupplier.get();
}
+ public ServiceLock getServiceLock(){
+ return serviceLockSupplier.get();
+ }
+
@Override
public Ample getAmple() {
+ // in the impl of ServerAmpleImpl we can call getServiceLock() as needed
return new ServerAmpleImpl(this);
}
```
If we had something like that, then this code could be
```java
public Ample.TabletMutator mutateTablet(KeyExtent extent) {
TabletsMutator tmi = mutateTablets();
Ample.TabletMutator tabletMutator = tmi.mutateTablet(extent, context.getServiceLock());
((TabletMutatorBase) tabletMutator).setCloseAfterMutate(tmi);
return tabletMutator;
}
```
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2023451261
This might be a bug in earlier versions as well. Also, it's not clear that we need to store the srv:lock column... just that we need to check it in a mutation. I'm not sure how easy it is to have a constraint check a mutation, but then alter it to omit the field, or if this should be done by some other means, like sending the lock information in the RPC message as a separate parameter from the mutation data.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1566346821
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/CleanUpBulkImport.java:
##########
Review Comment:
The changes to this file could be reverted.
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/commit/CommitCompaction.java:
##########
Review Comment:
The changes to this file could be reverted.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2059296393
In latest IT build only the SplitMillionIT timed out, no other failures. SplitMillionIT timed out once for me locally, and succeeded twice. So I think it's just flaky. I have no other planned changes for this PR.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2027686361
I think the only test failing at this point is BigRootTabletIT. I've spent probably 4 hours trying to track down the issue, and I'm unable to figure it out. After the split on the metadata table happens, the Manager doesn't appear to be hosting the new metadata tablets.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#discussion_r1558263761
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java:
##########
@@ -558,6 +558,11 @@ public synchronized void unlock() throws InterruptedException, KeeperException {
LOG.debug("[{}] Deleting all at path {} due to unlock", vmLockPrefix, pathToDelete);
ZooUtil.recursiveDelete(zooKeeper, pathToDelete, NodeMissingPolicy.SKIP);
+ // Wait for the delete to happen on the server before exiting method
+ while (zooKeeper.exists(pathToDelete, null) != null) {
Review Comment:
Added log msg to indicate that we are waiting in 1eca1ac
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2052642718
Tracked down the cause of the BigRootTabletIT and fixed it in #4457. I think these changes were causing the server side scan buffer to only hold a single key/value. The code would take the last key from the buffer and create a range like `Range(lastKeyInBuffer, false, ...)` and reseek to get the next batch. This reseek would not skip the key and reread the same key again resulting in an infinite loop. I noticed the TGW was spamming the logs and never flushing any changes and realized it was in an infinite loop.
--
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
Re: [PR] Modified Tablet Mutator implementions to always update srv:lock [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4421:
URL: https://github.com/apache/accumulo/pull/4421#issuecomment-2052684304
> @keith-turner infinite loop is exactly what I was seeing. Thanks for tracking this down.
I spent many hours looking at this. It took me a while to figure out the TGW was in an infinite loop. I was like why is the TGW never flushing assignments?
--
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