-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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
HBASE-24380 : Provide WAL splitting journal logging #1860
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
|
||
abstract List<Path> close() throws IOException; | ||
abstract List<Path> close(MonitoredTask status) throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
@@ -52,15 +53,16 @@ public RecoveredEditsOutputSink(WALSplitter walSplitter, | |||
} | |||
|
|||
@Override | |||
public void append(EntryBuffers.RegionEntryBuffer buffer) throws IOException { | |||
public void append(EntryBuffers.RegionEntryBuffer buffer, MonitoredTask status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
@@ -86,26 +88,27 @@ private RecoveredEditsWriter getRecoveredEditsWriter(TableName tableName, byte[] | |||
} | |||
|
|||
@Override | |||
public List<Path> close() throws IOException { | |||
public List<Path> close(MonitoredTask status) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
* @return true when there is no error. | ||
*/ | ||
private boolean closeWriters() throws IOException { | ||
private boolean closeWriters(MonitoredTask status) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
@@ -83,25 +85,26 @@ public void append(EntryBuffers.RegionEntryBuffer buffer) throws IOException { | |||
} | |||
|
|||
@Override | |||
public List<Path> close() throws IOException { | |||
public List<Path> close(MonitoredTask status) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this context have access to the OutputSink's MonitoredTask field? Can we do that instead, so we don't have to change these method signatures?
@@ -57,7 +58,8 @@ public BoundedRecoveredEditsOutputSink(WALSplitter walSplitter, | |||
} | |||
|
|||
@Override | |||
public void append(EntryBuffers.RegionEntryBuffer buffer) throws IOException { | |||
public void append(EntryBuffers.RegionEntryBuffer buffer, MonitoredTask status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this context have access to the OutputSink's MonitoredTask field? Can we do that instead, so we don't have to change these method signatures?
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredEditsOutputSink.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredHFilesOutputSink.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor thing, otherwise ltgm
@@ -233,7 +251,10 @@ void writeRegionEntries(List<WAL.Entry> entries) throws IOException { | |||
incrementNanoTime(System.nanoTime() - startTime); | |||
} catch (IOException e) { | |||
e = e instanceof RemoteException ? ((RemoteException) e).unwrapRemoteException() : e; | |||
LOG.error(HBaseMarkers.FATAL, "Got while writing log entry to log", e); | |||
final String errorMsg = | |||
"Failed to write log entry " + currentWalEntry.toString() + " to log"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing null check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOException is thrown by writer.append(logEntry)
and when we reach there, we are quite certain that currentWalEntry
will be assigned some value for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this brings a good point, Exception catch should rather be at a place where it is needed, entire block doesn't need to be within try/catch if only single line of code is throwing specific Exception. Let me take care of this.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Picking back... |
Signed-off-by: Andrew Purtell <apurtell@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredEditsOutputSink.java hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredHFilesOutputSink.java hbase-server/src/main/java/org/apache/hadoop/hbase/wal/RecoveredEditsOutputSink.java
Signed-off-by: Andrew Purtell <apurtell@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredEditsOutputSink.java hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredHFilesOutputSink.java hbase-server/src/main/java/org/apache/hadoop/hbase/wal/RecoveredEditsOutputSink.java
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredEditsOutputSink.java hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredHFilesOutputSink.java hbase-server/src/main/java/org/apache/hadoop/hbase/wal/RecoveredEditsOutputSink.java
…ging) (apache#1860) (apache#1937) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Guanghao Zhang <zghao@apache.org> (cherry picked from commit d154c79) Change-Id: I4c1c975d3408005a76072ab971c0a5c1217f47cf
Sample output: