Skip to content
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.

Sign up for GitHub

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

(trivial) removes some hardcoded fate values #5243

Merged
merged 2 commits into from
Jan 13, 2025
Merged

(trivial) removes some hardcoded fate values #5243

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c660e2
Removes some hardcoded values
kevinrr888 Jan 10, 2025
d4f4643
cleanup
kevinrr888 Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions core/src/main/java/org/apache/accumulo/core/fate/Fate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ public Fate(T environment, FateStore<T> store, boolean runDeadResCleaner,
idleCountHistory.add(workQueue.getWaitingConsumerCount());
}
}
}, 3, 30, SECONDS));
}, getInitialDelay().toSeconds(), getPoolWatcherDelay().toSeconds(), SECONDS));
this.transactionExecutor = pool;

ScheduledExecutorService deadResCleanerExecutor = null;
Expand All @@ -466,8 +466,9 @@ public Fate(T environment, FateStore<T> store, boolean runDeadResCleaner,
// reservations held by dead processes, if they exist.
deadResCleanerExecutor = ThreadPools.getServerThreadPools().createScheduledExecutorService(1,
store.type() + "-dead-reservation-cleaner-pool");
ScheduledFuture<?> deadReservationCleaner = deadResCleanerExecutor.scheduleWithFixedDelay(
new DeadReservationCleaner(), 3, getDeadResCleanupDelay().toSeconds(), SECONDS);
ScheduledFuture<?> deadReservationCleaner =
deadResCleanerExecutor.scheduleWithFixedDelay(new DeadReservationCleaner(),
getInitialDelay().toSeconds(), getDeadResCleanupDelay().toSeconds(), SECONDS);
ThreadPools.watchCriticalScheduledTask(deadReservationCleaner);
}
this.deadResCleanerExecutor = deadResCleanerExecutor;
Expand All @@ -476,10 +477,18 @@ public Fate(T environment, FateStore<T> store, boolean runDeadResCleaner,
this.workFinder.start();
}

public Duration getInitialDelay() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the plan to allow overriding these methods? If not maybe they could be public and statically defined constants in Fate? Even if we want to keep the methods to allow overriding they could delegate to the statically defined constants (if you keep the methods they could be private). This way we are not creating new objects each time the method is called.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestions. We shouldn't allow overriding for initial delay, so I removed that. getDeadResCleanupDelay is already overridden in FastFate and I also plan to override getPoolWatcherDelay in FastFate as well in an upcoming PR, so left those methods but no longer create a new Duration each time.

return Duration.ofSeconds(3);
}

public Duration getDeadResCleanupDelay() {
return Duration.ofMinutes(3);
}

public Duration getPoolWatcherDelay() {
return Duration.ofSeconds(30);
}

// get a transaction id back to the requester before doing any work
public FateId startTransaction() {
return store.create();
Expand Down
12 changes: 6 additions & 6 deletions test/src/main/java/org/apache/accumulo/test/fate/FateIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ protected void testTransactionStatus(FateStore<TestEnv> store, ServerContext sct
try {

// Wait for the transaction runner to be scheduled.
Thread.sleep(3000);
Thread.sleep(fate.getInitialDelay().toMillis() * 2);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

greater than the delay to avoid potential timing issues


callStarted = new CountDownLatch(1);
finishCall = new CountDownLatch(1);
Expand Down Expand Up @@ -295,7 +295,7 @@ protected void testCancelWhileNew(FateStore<TestEnv> store, ServerContext sctx)
try {

// Wait for the transaction runner to be scheduled.
Thread.sleep(3000);
Thread.sleep(fate.getInitialDelay().toMillis() * 2);

callStarted = new CountDownLatch(1);
finishCall = new CountDownLatch(1);
Expand Down Expand Up @@ -330,7 +330,7 @@ protected void testCancelWhileSubmittedAndRunning(FateStore<TestEnv> store, Serv
try {

// Wait for the transaction runner to be scheduled.
Thread.sleep(3000);
Thread.sleep(fate.getInitialDelay().toMillis() * 2);

callStarted = new CountDownLatch(1);
finishCall = new CountDownLatch(1);
Expand Down Expand Up @@ -365,7 +365,7 @@ protected void testCancelWhileInCall(FateStore<TestEnv> store, ServerContext sct
try {

// Wait for the transaction runner to be scheduled.
Thread.sleep(3000);
Thread.sleep(fate.getInitialDelay().toMillis() * 2);

callStarted = new CountDownLatch(1);
finishCall = new CountDownLatch(1);
Expand Down Expand Up @@ -402,7 +402,7 @@ protected void testDeferredOverflow(FateStore<TestEnv> store, ServerContext sctx
try {

// Wait for the transaction runner to be scheduled.
Thread.sleep(3000);
Thread.sleep(fate.getInitialDelay().toMillis() * 2);

DeferredTestRepo.executedCalls.set(0);
// Initialize the repo to have a delay of 30 seconds
Expand Down Expand Up @@ -479,7 +479,7 @@ protected void testRepoFails(FateStore<TestEnv> store, ServerContext sctx) throw
try {

// Wait for the transaction runner to be scheduled.
Thread.sleep(3000);
Thread.sleep(fate.getInitialDelay().toMillis() * 2);

List<String> expectedUndoOrder = List.of("OP3", "OP2", "OP1");
/*
Expand Down
Loading