keith-turner commented on code in PR #4524:
URL: https://github.com/apache/accumulo/pull/4524#discussion_r1702267741
##########
server/manager/src/main/java/org/apache/accumulo/manager/metrics/fate/meta/MetaFateMetrics.java:
##########
@@ -59,7 +60,8 @@ public void registerMetrics(MeterRegistry registry) {
@Override
protected ReadOnlyFateStore<FateMetrics<MetaFateMetricValues>>
buildStore(ServerContext context) {
try {
- return new MetaFateStore<>(getFateRootPath(context),
context.getZooReaderWriter());
+ return new MetaFateStore<>(getFateRootPath(context),
context.getZooReaderWriter(),
+ AbstractFateStore.createDummyLockID(), null);
Review Comment:
This store is being created for metrics, so it may be using it in a readonly
way and may never reserve. If it never reserves then it does not need a lock,
not even a dummy lock. Not something for this PR, but for this case I think we
want to somehow create a store obj where attempts to reserve will be non
functional and throw an exception.
##########
core/src/main/java/org/apache/accumulo/core/fate/AbstractFateStore.java:
##########
@@ -67,27 +76,36 @@ public FateId fromTypeAndKey(FateInstanceType instanceType,
FateKey fateKey) {
}
};
- protected final Set<FateId> reserved;
+ // The ZooKeeper lock for the process that's running this store instance
+ protected final ZooUtil.LockID lockID;
+ protected final Predicate<ZooUtil.LockID> isLockHeld;
protected final Map<FateId,NanoTime> deferred;
+ protected final FateIdGenerator fateIdGenerator;
private final int maxDeferred;
private final AtomicBoolean deferredOverflow = new AtomicBoolean();
- private final FateIdGenerator fateIdGenerator;
-
- // This is incremented each time a transaction was unreserved that was non
new
- protected final SignalCount unreservedNonNewCount = new SignalCount();
// This is incremented each time a transaction is unreserved that was
runnable
- protected final SignalCount unreservedRunnableCount = new SignalCount();
+ private final SignalCount unreservedRunnableCount = new SignalCount();
+
+ // Keeps track of the number of concurrent callers to waitForStatusChange()
+ private final AtomicInteger concurrentStatusChangeCallers = new
AtomicInteger(0);
public AbstractFateStore() {
- this(DEFAULT_MAX_DEFERRED, DEFAULT_FATE_ID_GENERATOR);
+ this(null, null, DEFAULT_MAX_DEFERRED, DEFAULT_FATE_ID_GENERATOR);
}
- public AbstractFateStore(int maxDeferred, FateIdGenerator fateIdGenerator) {
+ public AbstractFateStore(ZooUtil.LockID lockID, Predicate<ZooUtil.LockID>
isLockHeld) {
+ this(lockID, isLockHeld, DEFAULT_MAX_DEFERRED, DEFAULT_FATE_ID_GENERATOR);
+ }
+
+ public AbstractFateStore(ZooUtil.LockID lockID, Predicate<ZooUtil.LockID>
isLockHeld,
+ int maxDeferred, FateIdGenerator fateIdGenerator) {
this.maxDeferred = maxDeferred;
this.fateIdGenerator = Objects.requireNonNull(fateIdGenerator);
- this.reserved = new HashSet<>();
- this.deferred = new HashMap<>();
+ this.deferred = Collections.synchronizedMap(new HashMap<>());
+ this.lockID = Objects.requireNonNullElseGet(lockID,
AbstractFateStore::createDummyLockID);
Review Comment:
ok sounds good. I am working though comments and resolving them. Left some
comments open that seemed like they needed a follow on issue. Once issues are
opened can resolve those comments. We may need an issue about making the fate
utils get a lock.
##########
core/src/main/java/org/apache/accumulo/core/fate/user/RowFateStatusFilter.java:
##########
@@ -53,19 +56,28 @@ public void init(SortedKeyValueIterator<Key,Value> source,
Map<String,String> op
}
@Override
- public boolean accept(Key k, Value v) {
- var tstatus = ReadOnlyFateStore.TStatus.valueOf(v.toString());
- return valuesToAccept.contains(tstatus);
+ protected boolean filter(Text currentRow, List<Key> keys, List<Value>
values) {
+ for (int i = 0; i < keys.size(); i++) {
+ Key key = keys.get(i);
+ if (key.getColumnQualifier()
+ .equals(FateSchema.TxColumnFamily.STATUS_COLUMN.getColumnQualifier())
Review Comment:
```suggestion
if (FateSchema.TxColumnFamily.STATUS_COLUMN.hasColumns(key)
```
##########
core/src/main/java/org/apache/accumulo/core/fate/AbstractFateStore.java:
##########
@@ -57,8 +57,8 @@
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
// TODO 4131 should probably add support to AbstractFateStore, MetaFateStore,
-// and UserFateStore to accept null lockID and zooCache (maybe make these
fields
-// Optional<>). This could replace the current createDummyLockID(). This
support
+// and UserFateStore to accept null lockID (maybe make this field Optional<>).
Review Comment:
When I have a PR w/ a lot of TODOs sometimes I will try to open an issue for
each TODO and remove it before merging. Could do that for this PR but do not
have to, if not doing it then it would be good to use a more unique TODO which
I think is being done here? Do all of the TODOs have the 4131 suffix? If so
then it will be easy to find them with grep. Then we just need an open issue
that records the fact that all `TODO 4131` need to be done.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
