[GitHub] [hbase] saintstack commented on a change in pull request #1955: HBASE-24616 Remove BoundedRecoveredHFilesOutputSink dependency on a T…
saintstack commented on a change in pull request #1955:
URL: https://github.com/apache/hbase/pull/1955#discussion_r445765597
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredHFilesOutputSink.java
##
@@ -191,50 +186,22 @@ public boolean keepRegionEvent(Entry entry) {
return false;
}
+ /**
+ * @return Returns a base HFile without compressions or encodings; good
enough for recovery
Review comment:
bq. Actually the timeout can be set per rpc request on the same
connection.
You are right. Could try this later.
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
[GitHub] [hbase] saintstack commented on a change in pull request #1955: HBASE-24616 Remove BoundedRecoveredHFilesOutputSink dependency on a T…
saintstack commented on a change in pull request #1955:
URL: https://github.com/apache/hbase/pull/1955#discussion_r444337252
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredHFilesOutputSink.java
##
@@ -191,50 +186,22 @@ public boolean keepRegionEvent(Entry entry) {
return false;
}
+ /**
+ * @return Returns a base HFile without compressions or encodings; good
enough for recovery
+ * given hfile has metadata on how it was written.
+ */
private StoreFileWriter createRecoveredHFileWriter(TableName tableName,
String regionName,
long seqId, String familyName, boolean isMetaTable) throws IOException {
Path outputDir =
WALSplitUtil.tryCreateRecoveredHFilesDir(walSplitter.rootFS, walSplitter.conf,
tableName, regionName, familyName);
StoreFileWriter.Builder writerBuilder =
new StoreFileWriter.Builder(walSplitter.conf, CacheConfig.DISABLED,
walSplitter.rootFS)
.withOutputDir(outputDir);
-
-TableDescriptor tableDesc =
-tableDescCache.computeIfAbsent(tableName, t -> getTableDescriptor(t));
-if (tableDesc == null) {
- throw new IOException("Failed to get table descriptor for table " +
tableName);
-}
-ColumnFamilyDescriptor cfd =
tableDesc.getColumnFamily(Bytes.toBytesBinary(familyName));
-HFileContext hFileContext = createFileContext(cfd, isMetaTable);
-return
writerBuilder.withFileContext(hFileContext).withBloomType(cfd.getBloomFilterType())
-.build();
- }
-
- private HFileContext createFileContext(ColumnFamilyDescriptor cfd, boolean
isMetaTable)
- throws IOException {
-return new HFileContextBuilder().withCompression(cfd.getCompressionType())
-.withChecksumType(HStore.getChecksumType(walSplitter.conf))
-.withBytesPerCheckSum(HStore.getBytesPerChecksum(walSplitter.conf))
-
.withBlockSize(cfd.getBlocksize()).withCompressTags(cfd.isCompressTags())
-.withDataBlockEncoding(cfd.getDataBlockEncoding()).withCellComparator(
- isMetaTable ? CellComparatorImpl.META_COMPARATOR :
CellComparatorImpl.COMPARATOR)
-.build();
- }
-
- private TableDescriptor getTableDescriptor(TableName tableName) {
-if (walSplitter.rsServices != null) {
- try {
-return
walSplitter.rsServices.getConnection().getAdmin().getDescriptor(tableName);
- } catch (IOException e) {
-LOG.warn("Failed to get table descriptor for {}", tableName, e);
- }
-}
-LOG.info("Failed getting {} table descriptor from master; trying local",
tableName);
-try {
- return walSplitter.tableDescriptors.get(tableName);
Review comment:
Sorry, I get what you are saying now. You added this. Let me undo it.
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
[GitHub] [hbase] saintstack commented on a change in pull request #1955: HBASE-24616 Remove BoundedRecoveredHFilesOutputSink dependency on a T…
saintstack commented on a change in pull request #1955:
URL: https://github.com/apache/hbase/pull/1955#discussion_r444302316
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredHFilesOutputSink.java
##
@@ -191,50 +186,22 @@ public boolean keepRegionEvent(Entry entry) {
return false;
}
+ /**
+ * @return Returns a base HFile without compressions or encodings; good
enough for recovery
Review comment:
Asking the Master for the table descriptor first and if it fails, read
the fs is what we had before this patch. Problem was that default configuration
had it that the RPC query wasn't timing out before the below happened:
2020-06-18 19:53:54,175 ERROR [main] master.HMasterCommandLine: Master
exiting
java.lang.RuntimeException: Master not initialized after 20ms
I could dick around w/ timings inside here so RPC timed out sooner. It uses
the server's Connection. I suppose I could make a new Connection everytime we
want to query a Table schema and configure it to try once only but that is a
lot of work to do at recovery time.
On the compactions, don't we usually pick up the small files first?
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
[GitHub] [hbase] saintstack commented on a change in pull request #1955: HBASE-24616 Remove BoundedRecoveredHFilesOutputSink dependency on a T…
saintstack commented on a change in pull request #1955:
URL: https://github.com/apache/hbase/pull/1955#discussion_r444297274
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/BoundedRecoveredHFilesOutputSink.java
##
@@ -191,50 +186,22 @@ public boolean keepRegionEvent(Entry entry) {
return false;
}
+ /**
+ * @return Returns a base HFile without compressions or encodings; good
enough for recovery
+ * given hfile has metadata on how it was written.
+ */
private StoreFileWriter createRecoveredHFileWriter(TableName tableName,
String regionName,
long seqId, String familyName, boolean isMetaTable) throws IOException {
Path outputDir =
WALSplitUtil.tryCreateRecoveredHFilesDir(walSplitter.rootFS, walSplitter.conf,
tableName, regionName, familyName);
StoreFileWriter.Builder writerBuilder =
new StoreFileWriter.Builder(walSplitter.conf, CacheConfig.DISABLED,
walSplitter.rootFS)
.withOutputDir(outputDir);
-
-TableDescriptor tableDesc =
-tableDescCache.computeIfAbsent(tableName, t -> getTableDescriptor(t));
-if (tableDesc == null) {
- throw new IOException("Failed to get table descriptor for table " +
tableName);
-}
-ColumnFamilyDescriptor cfd =
tableDesc.getColumnFamily(Bytes.toBytesBinary(familyName));
-HFileContext hFileContext = createFileContext(cfd, isMetaTable);
-return
writerBuilder.withFileContext(hFileContext).withBloomType(cfd.getBloomFilterType())
-.build();
- }
-
- private HFileContext createFileContext(ColumnFamilyDescriptor cfd, boolean
isMetaTable)
- throws IOException {
-return new HFileContextBuilder().withCompression(cfd.getCompressionType())
-.withChecksumType(HStore.getChecksumType(walSplitter.conf))
-.withBytesPerCheckSum(HStore.getBytesPerChecksum(walSplitter.conf))
-
.withBlockSize(cfd.getBlocksize()).withCompressTags(cfd.isCompressTags())
-.withDataBlockEncoding(cfd.getDataBlockEncoding()).withCellComparator(
- isMetaTable ? CellComparatorImpl.META_COMPARATOR :
CellComparatorImpl.COMPARATOR)
-.build();
- }
-
- private TableDescriptor getTableDescriptor(TableName tableName) {
-if (walSplitter.rsServices != null) {
- try {
-return
walSplitter.rsServices.getConnection().getAdmin().getDescriptor(tableName);
- } catch (IOException e) {
-LOG.warn("Failed to get table descriptor for {}", tableName, e);
- }
-}
-LOG.info("Failed getting {} table descriptor from master; trying local",
tableName);
-try {
- return walSplitter.tableDescriptors.get(tableName);
Review comment:
Good question.
Looking at it, it usually gets them from the hosting Server, not by RPC to
Master so there it should not suffer the issue we see here.
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
[GitHub] [hbase] saintstack commented on a change in pull request #1955: HBASE-24616 Remove BoundedRecoveredHFilesOutputSink dependency on a T…
saintstack commented on a change in pull request #1955:
URL: https://github.com/apache/hbase/pull/1955#discussion_r444295419
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/RSProcedureHandler.java
##
@@ -48,7 +48,7 @@ public void process() {
try {
callable.call();
} catch (Throwable t) {
- LOG.error("Error when call RSProcedureCallable: ", t);
+ LOG.error("pid=" + this.procId, t);
Review comment:
I changed it so that it logs the pid same way we do it every where else.
The thread/class/level already provides what we add here. I just removed
redundancy.
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
[GitHub] [hbase] saintstack commented on a change in pull request #1955: HBASE-24616 Remove BoundedRecoveredHFilesOutputSink dependency on a T…
saintstack commented on a change in pull request #1955:
URL: https://github.com/apache/hbase/pull/1955#discussion_r444294454
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##
@@ -2037,8 +2037,7 @@ private void startServices() throws IOException {
this.splitLogWorker = new SplitLogWorker(sinkConf, this,
this, walFactory);
splitLogWorker.start();
-} else {
- LOG.warn("SplitLogWorker Service NOT started; CoordinatedStateManager is
null");
+ LOG.debug("SplitLogWorker started");
Review comment:
Previous, we used to log when we did NOT start the splitlogworker
service. This goes against our usual pattern of only logging started services
(logging all the things we did not start would be an endless list -- smile)
I changed the log here so that it conforms with our usual pattern.
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
