anoopsjohn commented on a change in pull request #2021:
URL: https://github.com/apache/hbase/pull/2021#discussion_r459857809
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##########
@@ -58,31 +58,31 @@
protected static final String WAL_ROLL_PERIOD_KEY =
"hbase.regionserver.logroll.period";
- protected final ConcurrentMap<WAL, Boolean> walNeedsRoll = new
ConcurrentHashMap<>();
+ protected final ConcurrentMap<WAL, RollController> wals = new
ConcurrentHashMap<>();
protected final T abortable;
- private volatile long lastRollTime = System.currentTimeMillis();
// Period to roll log.
private final long rollPeriod;
private final int threadWakeFrequency;
// The interval to check low replication on hlog's pipeline
- private long checkLowReplicationInterval;
+ private final long checkLowReplicationInterval;
private volatile boolean running = true;
public void addWAL(WAL wal) {
// check without lock first
- if (walNeedsRoll.containsKey(wal)) {
+ if (wals.containsKey(wal)) {
return;
}
// this is to avoid race between addWAL and requestRollAll.
synchronized (this) {
- if (walNeedsRoll.putIfAbsent(wal, Boolean.FALSE) == null) {
+ if (wals.putIfAbsent(wal, new RollController(wal)) == null) {
wal.registerWALActionsListener(new WALActionsListener() {
@Override
public void logRollRequested(WALActionsListener.RollRequestReason
reason) {
// TODO logs will contend with each other here, replace with e.g.
DelayedQueue
synchronized (AbstractWALRoller.this) {
- walNeedsRoll.put(wal, Boolean.TRUE);
+ RollController controller = wals.computeIfAbsent(wal, rc -> new
RollController(wal));
Review comment:
Good Q. In fact I also thought when reviewed this. Ideally speaking we
should get the addWAL call 1st which will add the instance to the Map. When we
get call here the wal should be in the map already. But if u see the cur impl,
there is no such contract enforcing. It just add the WAL with True value. So
believe while making patch, @WenFeiYi went with similar lines.
We can consider this.. Need to see any chance we get a rollReq before
adding.. While RS start, we do some rollReq on WALs.. This introduced some bug
in the past. We need to see that closely.. If we can confirm that we can
add that contract enforcing and so what u suggested. I would say add a TODO
here and raise another issue. This went through multiple cycles of changes. :-)
U ok Viraj?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
