dnhatn commented on a change in pull request #1925:
URL: https://github.com/apache/lucene-solr/pull/1925#discussion_r496348288
##########
File path:
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThread.java
##########
@@ -284,7 +283,7 @@ FrozenBufferedUpdates prepareFlush() {
/** Flush all pending docs to a new segment */
FlushedSegment flush(DocumentsWriter.FlushNotifications flushNotifications)
throws IOException {
- assert flushPending.get() == Boolean.TRUE;
+ assert state == State.FLUSHING;
Review comment:
Maybe add the current state to the assertion.
##########
File path:
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThread.java
##########
@@ -169,6 +167,7 @@ long updateDocuments(Iterable<? extends Iterable<? extends
IndexableField>> docs
try {
testPoint("DocumentsWriterPerThread addDocuments start");
assert abortingException == null: "DWPT has hit aborting exception but
is still indexing";
+ assert state == State.ACTIVE || state == State.FLUSH_PENDING : "Illegal
state: " + state + " must be ACTIVE of FLUSH_PENDING";
Review comment:
nit: of -> or
##########
File path:
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java
##########
@@ -157,25 +157,18 @@ private boolean updatePeaks(long delta) {
}
DocumentsWriterPerThread doAfterDocument(DocumentsWriterPerThread perThread,
boolean isUpdate) {
- final long delta = perThread.getCommitLastBytesUsedDelta();
+ final long delta = perThread.commitLastBytesUsed();
synchronized (this) {
- // we need to commit this under lock but calculate it outside of the
lock to minimize the time this lock is held
- // per document. The reason we update this under lock is that we mark
DWPTs as pending without acquiring it's
- // lock in #setFlushPending and this also reads the committed bytes and
modifies the flush/activeBytes.
- // In the future we can clean this up to be more intuitive.
- perThread.commitLastBytesUsed(delta);
try {
/*
* We need to differentiate here if we are pending since
setFlushPending
* moves the perThread memory to the flushBytes and we could be set to
* pending during a delete
*/
- if (perThread.isFlushPending()) {
- flushBytes += delta;
- assert updatePeaks(delta);
- } else {
- activeBytes += delta;
- assert updatePeaks(delta);
+ activeBytes += delta;
+ assert updatePeaks(delta);
+ if (perThread.isFlushPending() == false) {
+ assert perThread.getState() == DocumentsWriterPerThread.State.ACTIVE
: "expected ACTIVE state but was: " + perThread.getState();
Review comment:
Do we still need `isFlushPending` method? Should we compare the state of
`perThread` to ACTIVE or FLUSH_PENDING instead.
##########
File path:
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThread.java
##########
@@ -593,9 +568,61 @@ void unlock() {
}
/**
- * Returns <code>true</code> iff this DWPT has been flushed
+ * Returns the DWPTs current state.
*/
- boolean hasFlushed() {
- return hasFlushed.get() == Boolean.TRUE;
+ State getState() {
+ return state;
}
+
+ /**
+ * Transitions the DWPT to the given state of fails if the transition is
invalid.
+ * @throws IllegalStateException if the given state can not be transitioned
to.
+ */
+ synchronized void transitionTo(State state) {
+ if (state.canTransitionFrom(this.state) == false) {
+ throw new IllegalStateException("Can't transition from " + this.state +
" to " + state);
+ }
+ assert state.mustHoldLock == false || isHeldByCurrentThread() : "illegal
state: " + state + " lock is held: " + isHeldByCurrentThread();
+ this.state = state;
+ }
+
+ /**
+ * Internal DWPT State.
+ */
+ enum State {
+ /**
+ * Default states when a DWPT is initialized and ready to index documents.
+ */
+ ACTIVE(null, true),
+ /**
+ * The DWPT can still index documents but should be moved to FLUSHING
state as soon as possible.
+ * Transitions to this state can be done concurrently while another thread
is actively indexing into this DWPT.
+ */
+ FLUSH_PENDING(ACTIVE, false),
+ /**
+ * The DWPT should not receive any further documents and is current
flushing or queued up for flushing.
+ */
+ FLUSHING(FLUSH_PENDING, true),
+ /**
+ * The DWPT has been flushed and is ready to be garbage collected.
+ */
+ FLUSHED(FLUSHING, false);
+
+ private final State previousState;
+ final boolean mustHoldLock; // only for asserts
Review comment:
This can be private?
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
