Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar merged PR #7922: URL: https://github.com/apache/hbase/pull/7922 -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on PR #7922: URL: https://github.com/apache/hbase/pull/7922#issuecomment-4137705570 I've fun the failing unit test 3 times locally without an error, so I go ahead and merge this patch. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7922:
URL: https://github.com/apache/hbase/pull/7922#discussion_r2996369693
##
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java:
##
@@ -99,13 +100,14 @@ public static void manageActiveClusterIdFile(boolean
readOnlyEnabled, MasterFile
"Failed to delete active cluster file: {}. "
+ "Read-only flag will be updated, but file system state is
inconsistent.",
activeClusterFile, e);
+} catch (DeserializationException e) {
+ LOG.error("Failed to deserialize ActiveClusterSuffix from file {}",
activeClusterFile, e);
}
} else {
// DISABLING READ-ONLY (true -> false), create the active cluster file
id file
int wait =
mfs.getConfiguration().getInt(HConstants.THREAD_WAKE_FREQUENCY, 10 * 1000);
if (!fs.exists(activeClusterFile)) {
- FSUtils.setActiveClusterSuffix(fs, rootDir,
mfs.computeAndSetSuffixFileDataToWrite(),
-wait);
+ FSUtils.setActiveClusterSuffix(fs, rootDir,
mfs.getActiveClusterSuffix(), wait);
} else {
LOG.debug("Active cluster file already exists at: {}. No need to
create it again.",
activeClusterFile);
Review Comment:
I see an implementation gap here as well. Being the file present alone
doesn't guarantee that we could enable read-write mode. We need to make sure
that suffix file contains the suffix of this cluster.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7922: URL: https://github.com/apache/hbase/pull/7922#discussion_r2996362326 ## hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java: ## Review Comment: I think you're right. Switching to non-readonly mode or in other words enabling write operation in the cluster is a risky operation and we should make sure that it's done properly and completely. If we're unable to write the lock file for some reason, we should deny the entire mode switch. On the other hand, we need to make progress and fixing such kind of things is not scope of this ticket, so I suggest to submit this first and open a new ticket to confirm and address the above mentioned problems. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7922: URL: https://github.com/apache/hbase/pull/7922#discussion_r2932155128 ## hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java: ## Review Comment: @anmolnar On the contrary to above discussion, I think we should throw exception here as this case will cause data corruption because you are not able to create the cluster file but you now cluster is active. I also think, even if this throw exception, then master will not change its readonly state. But how can we notify Region's OnConfigurationChange to abort changing the state because I think calls to configuration observers are executed in parallel. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7922:
URL: https://github.com/apache/hbase/pull/7922#discussion_r2975188841
##
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java:
##
@@ -84,13 +85,14 @@ public static void manageActiveClusterIdFile(boolean
readOnlyEnabled, MasterFile
"Failed to delete active cluster file: {}. "
+ "Read-only flag will be updated, but file system state is
inconsistent.",
activeClusterFile, e);
+} catch (DeserializationException e) {
+ LOG.error("Failed to deserialize ActiveClusterSuffix from file {}",
activeClusterFile, e);
Review Comment:
Got you. I can accept that.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 closed pull request #7857: HBASE-29958 Improve log messages URL: https://github.com/apache/hbase/pull/7857 -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on PR #7857: URL: https://github.com/apache/hbase/pull/7857#issuecomment-4086328208 @sharmaar12 Can we close this PR? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2932206089
##
hbase-client/src/main/java/org/apache/hadoop/hbase/ActiveClusterSuffix.java:
##
@@ -31,18 +34,40 @@
*/
@InterfaceAudience.Private
public class ActiveClusterSuffix {
- private final String active_cluster_suffix;
+ private final String cluster_id;
+ private final String suffix;
/**
* New ActiveClusterSuffix.
*/
- public ActiveClusterSuffix(final String cs) {
-this.active_cluster_suffix = cs;
+ public ActiveClusterSuffix(final String cs, final String suffix) {
+this.cluster_id = cs;
+this.suffix = suffix;
}
- public String getActiveClusterSuffix() {
-return active_cluster_suffix;
+ public ActiveClusterSuffix(final String input) {
Review Comment:
We will be covering this test in this PR:
https://github.com/apache/hbase/pull/7923/changes#r2932171031
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7922:
URL: https://github.com/apache/hbase/pull/7922#discussion_r2932134953
##
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java:
##
@@ -84,13 +85,14 @@ public static void manageActiveClusterIdFile(boolean
readOnlyEnabled, MasterFile
"Failed to delete active cluster file: {}. "
+ "Read-only flag will be updated, but file system state is
inconsistent.",
activeClusterFile, e);
+} catch (DeserializationException e) {
+ LOG.error("Failed to deserialize ActiveClusterSuffix from file {}",
activeClusterFile, e);
Review Comment:
@anmolnar I believe we just log the error for any of these exception is
because the cluster is read only which means it is expected to delete the file
if it exists and cluster is the owner of that file, but if the file does not
exists or it is not the owner then we can safely ignore it, because for
read-only we are not mutating any data so safe to start the cluster.
Similarly I think if we can't deserialize it then deletion will not happen,
but cluster can start in read-only mode. Anyways this case needs manual
intervention because of cluster ID file corruption.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7922:
URL: https://github.com/apache/hbase/pull/7922#discussion_r2927408814
##
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java:
##
@@ -84,13 +85,14 @@ public static void manageActiveClusterIdFile(boolean
readOnlyEnabled, MasterFile
"Failed to delete active cluster file: {}. "
+ "Read-only flag will be updated, but file system state is
inconsistent.",
activeClusterFile, e);
+} catch (DeserializationException e) {
+ LOG.error("Failed to deserialize ActiveClusterSuffix from file {}",
activeClusterFile, e);
Review Comment:
I'm not sure about catching the exception here. We just log the error and
proceed. Isn't that a problem?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2924710242
##
hbase-client/src/main/java/org/apache/hadoop/hbase/ActiveClusterSuffix.java:
##
@@ -31,18 +34,40 @@
*/
@InterfaceAudience.Private
public class ActiveClusterSuffix {
- private final String active_cluster_suffix;
+ private final String cluster_id;
+ private final String suffix;
/**
* New ActiveClusterSuffix.
*/
- public ActiveClusterSuffix(final String cs) {
-this.active_cluster_suffix = cs;
+ public ActiveClusterSuffix(final String cs, final String suffix) {
+this.cluster_id = cs;
+this.suffix = suffix;
}
- public String getActiveClusterSuffix() {
-return active_cluster_suffix;
+ public ActiveClusterSuffix(final String input) {
Review Comment:
I think we should implement regex validation somewhere to make sure that
suffix only contains valid characters. We can do this in a separate ticket.
Something like: `^[a-zA-Z0-9]`
What we should do in this ticket though is to add unit test to cover the
split logic.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2923562062
##
hbase-client/src/main/java/org/apache/hadoop/hbase/ActiveClusterSuffix.java:
##
@@ -31,18 +34,40 @@
*/
@InterfaceAudience.Private
public class ActiveClusterSuffix {
- private final String active_cluster_suffix;
+ private final String cluster_id;
+ private final String suffix;
/**
* New ActiveClusterSuffix.
*/
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2923554775
##
hbase-client/src/main/java/org/apache/hadoop/hbase/ActiveClusterSuffix.java:
##
@@ -31,18 +34,40 @@
*/
@InterfaceAudience.Private
public class ActiveClusterSuffix {
- private final String active_cluster_suffix;
+ private final String cluster_id;
+ private final String suffix;
/**
* New ActiveClusterSuffix.
*/
- public ActiveClusterSuffix(final String cs) {
-this.active_cluster_suffix = cs;
+ public ActiveClusterSuffix(final String cs, final String suffix) {
+this.cluster_id = cs;
+this.suffix = suffix;
}
- public String getActiveClusterSuffix() {
-return active_cluster_suffix;
+ public ActiveClusterSuffix(final String input) {
Review Comment:
Good point. I have added to code to split on the first : only.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2923496549
##
hbase-client/src/main/java/org/apache/hadoop/hbase/ActiveClusterSuffix.java:
##
@@ -31,18 +34,40 @@
*/
@InterfaceAudience.Private
public class ActiveClusterSuffix {
- private final String active_cluster_suffix;
+ private final String cluster_id;
+ private final String suffix;
/**
* New ActiveClusterSuffix.
*/
- public ActiveClusterSuffix(final String cs) {
-this.active_cluster_suffix = cs;
+ public ActiveClusterSuffix(final String cs, final String suffix) {
+this.cluster_id = cs;
+this.suffix = suffix;
}
- public String getActiveClusterSuffix() {
-return active_cluster_suffix;
+ public ActiveClusterSuffix(final String input) {
+String[] parts = input.split(":");
+this.cluster_id = parts[0];
+if (parts.length > 1) {
+ this.suffix = parts[1];
+} else {
+ this.suffix = "";
+}
+ }
+
+ public static ActiveClusterSuffix fromConfig(Configuration conf, ClusterId
clusterId) {
+return new ActiveClusterSuffix(clusterId.toString(),
conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
+ HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE));
Review Comment:
getSuffixFromConfig() will give you full string containing
: but here we just want hence we can't change this
way.
Also getSuffixFromConfig has been removed post Andor's changes.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
kgeisz commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2920280655
##
hbase-client/src/main/java/org/apache/hadoop/hbase/ActiveClusterSuffix.java:
##
@@ -31,18 +34,40 @@
*/
@InterfaceAudience.Private
public class ActiveClusterSuffix {
- private final String active_cluster_suffix;
+ private final String cluster_id;
+ private final String suffix;
/**
* New ActiveClusterSuffix.
*/
- public ActiveClusterSuffix(final String cs) {
-this.active_cluster_suffix = cs;
+ public ActiveClusterSuffix(final String cs, final String suffix) {
+this.cluster_id = cs;
+this.suffix = suffix;
}
- public String getActiveClusterSuffix() {
-return active_cluster_suffix;
+ public ActiveClusterSuffix(final String input) {
+String[] parts = input.split(":");
+this.cluster_id = parts[0];
+if (parts.length > 1) {
+ this.suffix = parts[1];
+} else {
+ this.suffix = "";
+}
+ }
+
+ public static ActiveClusterSuffix fromConfig(Configuration conf, ClusterId
clusterId) {
+return new ActiveClusterSuffix(clusterId.toString(),
conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
+ HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE));
Review Comment:
```suggestion
return new ActiveClusterSuffix(clusterId.toString(),
getSuffixFromConfig(conf));
```
##
hbase-client/src/main/java/org/apache/hadoop/hbase/ActiveClusterSuffix.java:
##
@@ -31,18 +34,40 @@
*/
@InterfaceAudience.Private
public class ActiveClusterSuffix {
- private final String active_cluster_suffix;
+ private final String cluster_id;
+ private final String suffix;
/**
* New ActiveClusterSuffix.
*/
- public ActiveClusterSuffix(final String cs) {
-this.active_cluster_suffix = cs;
+ public ActiveClusterSuffix(final String cs, final String suffix) {
+this.cluster_id = cs;
+this.suffix = suffix;
}
- public String getActiveClusterSuffix() {
-return active_cluster_suffix;
+ public ActiveClusterSuffix(final String input) {
Review Comment:
What if the user puts a `:` in the cluster suffix? I think we should either
not allow the user to put a `:` in the suffix or parse on just the first `:`
(assuming cluster IDs never have a `:` in them).
##
hbase-client/src/main/java/org/apache/hadoop/hbase/ActiveClusterSuffix.java:
##
@@ -31,18 +34,40 @@
*/
@InterfaceAudience.Private
public class ActiveClusterSuffix {
- private final String active_cluster_suffix;
+ private final String cluster_id;
+ private final String suffix;
/**
* New ActiveClusterSuffix.
*/
Review Comment:
nit: remove blank line between docstring and constructor definition
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
Kota-SH commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2913364619
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
Review Comment:
In line#402, `File Suffix {} : Configured suffix {}`, adding some details
about which file and which configured suffix would be helpful
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2913348897
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
@@ -405,14 +405,15 @@ private void negotiateActiveClusterSuffixFile(long wait)
throws IOException {
this.activeClusterSuffix = acs;
} else {
// throw error
- LOG.info("rootdir {} : Active Cluster File Suffix {} ", rootdir,
acs);
+ LOG.info("rootdir {} : Read replica active cluster file suffix {} ",
rootdir, acs);
Review Comment:
Change this to:
`LOG.info("[Read-replica feature] Another cluster is running in active
(read-write) mode on this storage location. Active cluster ID: {}, This cluster
ID {}. Rootdir location {} ", acs, getSuffixFromConfig(), rootdir);`
Hope this helps.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7857: URL: https://github.com/apache/hbase/pull/7857#discussion_r2913310706 ## hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java: ## Review Comment: @Kota-SH Which config you are referring 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2913281110
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
@@ -405,14 +405,15 @@ private void negotiateActiveClusterSuffixFile(long wait)
throws IOException {
this.activeClusterSuffix = acs;
} else {
// throw error
- LOG.info("rootdir {} : Active Cluster File Suffix {} ", rootdir,
acs);
+ LOG.info("rootdir {} : Read replica active cluster file suffix {} ",
rootdir, acs);
throw new IOException("Cannot start master, because another cluster
is running in active "
-+ "(read-write) mode on this storage location. Active Cluster Id:
{} " + acs
-+ " This cluster Id: " + getClusterId());
++ "(read-write) mode on this storage location. Active Cluster Id:
" + acs
++ ", This cluster Id: "
++ new String(getSuffixFileDataToWrite(), StandardCharsets.UTF_8));
}
LOG.info(
- "This is the active cluster on this storage location, " + "File
Suffix {} : Suffix {} : ",
- acs, getActiveClusterSuffix());
+ "Read Replica Cluster: This is the active cluster on this storage
location with cluster id: {}",
+ new String(getSuffixFileDataToWrite(), StandardCharsets.UTF_8));
} catch (FileNotFoundException fnfe) {
// this is the active cluster, create active cluster suffix file if it
does not exist
FSUtils.setActiveClusterSuffix(fs, rootdir,
getSuffixFileDataToWrite(), wait);
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2913107694
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -92,7 +92,7 @@ public static TableName getDefaultNameOfMetaForReplica() {
public static TableName initializeHbaseMetaTableName(Configuration conf) {
String suffix_val = conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE);
-LOG.info("Meta table suffix value: {}", suffix_val);
+LOG.info("Read Replica Cluster suffix value: {}", suffix_val);
Review Comment:
Added this to debug log.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
Kota-SH commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2907436181
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -92,7 +92,7 @@ public static TableName getDefaultNameOfMetaForReplica() {
public static TableName initializeHbaseMetaTableName(Configuration conf) {
String suffix_val = conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE);
-LOG.info("Meta table suffix value: {}", suffix_val);
+LOG.info("Read Replica Cluster suffix value: {}", suffix_val);
Review Comment:
Yeah, it might be redundant at info level, we can move it to debug.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2907127873
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -92,7 +92,7 @@ public static TableName getDefaultNameOfMetaForReplica() {
public static TableName initializeHbaseMetaTableName(Configuration conf) {
String suffix_val = conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE);
-LOG.info("Meta table suffix value: {}", suffix_val);
+LOG.info("Read Replica Cluster suffix value: {}", suffix_val);
Review Comment:
...or move it to debug level with the "[Read-replica feature]" prefix that
you suggested.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2907098300
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -92,7 +92,7 @@ public static TableName getDefaultNameOfMetaForReplica() {
public static TableName initializeHbaseMetaTableName(Configuration conf) {
String suffix_val = conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE);
-LOG.info("Meta table suffix value: {}", suffix_val);
+LOG.info("Read Replica Cluster suffix value: {}", suffix_val);
Review Comment:
We might want to remove this log line completely. Meta table name is already
logged as:
```
2026-03-09T16:12:48,322 INFO [main] hbase.TableName: Meta table name:
hbase:meta
```
Which should be enough.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2907098300
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -92,7 +92,7 @@ public static TableName getDefaultNameOfMetaForReplica() {
public static TableName initializeHbaseMetaTableName(Configuration conf) {
String suffix_val = conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE);
-LOG.info("Meta table suffix value: {}", suffix_val);
+LOG.info("Read Replica Cluster suffix value: {}", suffix_val);
Review Comment:
We might want to remove this log line at all. Meta table name is already
logged as:
```
2026-03-09T16:12:48,322 INFO [main] hbase.TableName: Meta table name:
hbase:meta
```
Which should be enough.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
Kota-SH commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2906965299
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -92,7 +92,7 @@ public static TableName getDefaultNameOfMetaForReplica() {
public static TableName initializeHbaseMetaTableName(Configuration conf) {
String suffix_val = conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE);
-LOG.info("Meta table suffix value: {}", suffix_val);
+LOG.info("Read Replica Cluster suffix value: {}", suffix_val);
Review Comment:
Perhaps we could move this log into the else block (line 98)? That way it
only fires when the suffix exists. If not, adding a prefix like "[Read-replica
feature]" to the log line would also work well.
Since we’re initializing the meta table name here, I think having "meta
table name suffix" in the log would be a bit more informative, wdyt?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2906887537
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
@@ -405,14 +405,15 @@ private void negotiateActiveClusterSuffixFile(long wait)
throws IOException {
this.activeClusterSuffix = acs;
} else {
// throw error
- LOG.info("rootdir {} : Active Cluster File Suffix {} ", rootdir,
acs);
+ LOG.info("rootdir {} : Read replica active cluster file suffix {} ",
rootdir, acs);
throw new IOException("Cannot start master, because another cluster
is running in active "
-+ "(read-write) mode on this storage location. Active Cluster Id:
{} " + acs
-+ " This cluster Id: " + getClusterId());
++ "(read-write) mode on this storage location. Active Cluster Id:
" + acs
++ ", This cluster Id: "
++ new String(getSuffixFileDataToWrite(), StandardCharsets.UTF_8));
}
LOG.info(
- "This is the active cluster on this storage location, " + "File
Suffix {} : Suffix {} : ",
- acs, getActiveClusterSuffix());
+ "Read Replica Cluster: This is the active cluster on this storage
location with cluster id: {}",
Review Comment:
I like this approach.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2906881292
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -92,7 +92,7 @@ public static TableName getDefaultNameOfMetaForReplica() {
public static TableName initializeHbaseMetaTableName(Configuration conf) {
String suffix_val = conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE);
-LOG.info("Meta table suffix value: {}", suffix_val);
+LOG.info("Read Replica Cluster suffix value: {}", suffix_val);
Review Comment:
I'd like to connect the message to the feature somehow. What do you
recommend?
We're not implementing a big feature flag for the feature, so once this is
merged every HBase cluster will be either an active or a read-only cluster.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2906881292
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -92,7 +92,7 @@ public static TableName getDefaultNameOfMetaForReplica() {
public static TableName initializeHbaseMetaTableName(Configuration conf) {
String suffix_val = conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE);
-LOG.info("Meta table suffix value: {}", suffix_val);
+LOG.info("Read Replica Cluster suffix value: {}", suffix_val);
Review Comment:
I'd like to connect the message to the feature somehow. What do you
recommend?
We're not implementing a big feature flag for the feature, so once this is
merged every HBase cluster will be either and active or a read-only cluster.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
Kota-SH commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2906724291
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java:
##
@@ -616,14 +616,14 @@ public static ActiveClusterSuffix
getActiveClusterSuffix(FileSystem fs, Path roo
data = in.readUTF();
cs = new ActiveClusterSuffix(data);
} catch (EOFException eof) {
- LOG.warn("Active Cluster Suffix File {} is empty ", idPath);
+ LOG.warn("Read Replica Cluster id file {} is empty ", idPath);
} finally {
in.close();
}
}
return cs;
} else {
- throw new FileNotFoundException("Active Cluster Suffix File " + idPath +
" not found");
+ throw new FileNotFoundException("Read Replica Cluster id file " + idPath
+ " not found");
Review Comment:
Same here.
```suggestion
throw new FileNotFoundException("[Read-replica feature] Active Cluster
Suffix File " + idPath + " not found");
```
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
@@ -405,14 +405,15 @@ private void negotiateActiveClusterSuffixFile(long wait)
throws IOException {
this.activeClusterSuffix = acs;
} else {
// throw error
- LOG.info("rootdir {} : Active Cluster File Suffix {} ", rootdir,
acs);
+ LOG.info("rootdir {} : Read replica active cluster file suffix {} ",
rootdir, acs);
Review Comment:
This is a mismatch scenario, logging something like "Active cluster suffix
mismatch -- rootdir: {} but the file contains: {}, another cluster is running
in active" would be more helpful imo.
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java:
##
@@ -616,14 +616,14 @@ public static ActiveClusterSuffix
getActiveClusterSuffix(FileSystem fs, Path roo
data = in.readUTF();
cs = new ActiveClusterSuffix(data);
} catch (EOFException eof) {
- LOG.warn("Active Cluster Suffix File {} is empty ", idPath);
+ LOG.warn("Read Replica Cluster id file {} is empty ", idPath);
Review Comment:
```suggestion
LOG.warn("[Read Replica Feature] Active Cluster id file {} is
empty ", idPath);
```
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
Review Comment:
Detail about which file and which config in this log line would be more
helpful while debugging.
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
@@ -405,14 +405,15 @@ private void negotiateActiveClusterSuffixFile(long wait)
throws IOException {
this.activeClusterSuffix = acs;
} else {
// throw error
- LOG.info("rootdir {} : Active Cluster File Suffix {} ", rootdir,
acs);
+ LOG.info("rootdir {} : Read replica active cluster file suffix {} ",
rootdir, acs);
throw new IOException("Cannot start master, because another cluster
is running in active "
-+ "(read-write) mode on this storage location. Active Cluster Id:
{} " + acs
-+ " This cluster Id: " + getClusterId());
++ "(read-write) mode on this storage location. Active Cluster Id:
" + acs
++ ", This cluster Id: "
++ new String(getSuffixFileDataToWrite(), StandardCharsets.UTF_8));
}
LOG.info(
- "This is the active cluster on this storage location, " + "File
Suffix {} : Suffix {} : ",
- acs, getActiveClusterSuffix());
+ "Read Replica Cluster: This is the active cluster on this storage
location with cluster id: {}",
Review Comment:
```suggestion
"[Read-replica feature] This cluster is the active (read-write)
cluster on this shared storage location, cluster id: {}",
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
Kota-SH commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2906395325
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -92,7 +92,7 @@ public static TableName getDefaultNameOfMetaForReplica() {
public static TableName initializeHbaseMetaTableName(Configuration conf) {
String suffix_val = conf.get(HConstants.HBASE_META_TABLE_SUFFIX,
HConstants.HBASE_META_TABLE_SUFFIX_DEFAULT_VALUE);
-LOG.info("Meta table suffix value: {}", suffix_val);
+LOG.info("Read Replica Cluster suffix value: {}", suffix_val);
Review Comment:
This method is called by every cluster, active/replica. Labeling it "Read
Replica Cluster" in the log will cause confusion for standard (non-replica)
deployments.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on PR #7857: URL: https://github.com/apache/hbase/pull/7857#issuecomment-4025002359 @anmolnar Let me change accordingly. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on PR #7857: URL: https://github.com/apache/hbase/pull/7857#issuecomment-4024886406 > @anmolnar I am not much familiar with the nitty-gritty of this file based implementation as some other engineer worked on this but IIRC this may means that active.cluster.id.file may have been created without any data in it. I am not sure in which scenario this will happen. I mentioned already: > I restarted the active cluster when the active cluster suffix file was present already with the cluster id + blank suffix value: 0115d976-1306-44c7-b8b6-f2644f8867c9: -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2906412164
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
@@ -405,14 +405,15 @@ private void negotiateActiveClusterSuffixFile(long wait)
throws IOException {
this.activeClusterSuffix = acs;
} else {
// throw error
- LOG.info("rootdir {} : Active Cluster File Suffix {} ", rootdir,
acs);
+ LOG.info("rootdir {} : Read replica active cluster file suffix {} ",
rootdir, acs);
throw new IOException("Cannot start master, because another cluster
is running in active "
-+ "(read-write) mode on this storage location. Active Cluster Id:
{} " + acs
-+ " This cluster Id: " + getClusterId());
++ "(read-write) mode on this storage location. Active Cluster Id:
" + acs
++ ", This cluster Id: "
++ new String(getSuffixFileDataToWrite(), StandardCharsets.UTF_8));
}
LOG.info(
- "This is the active cluster on this storage location, " + "File
Suffix {} : Suffix {} : ",
- acs, getActiveClusterSuffix());
+ "Read Replica Cluster: This is the active cluster on this storage
location with cluster id: {}",
+ new String(getSuffixFileDataToWrite(), StandardCharsets.UTF_8));
} catch (FileNotFoundException fnfe) {
// this is the active cluster, create active cluster suffix file if it
does not exist
FSUtils.setActiveClusterSuffix(fs, rootdir,
getSuffixFileDataToWrite(), wait);
Review Comment:
Logging of this branch is also inaccurate. This is the point where we first
create the suffix file when file system is initialized in the active cluster
and no logs are emitted. The FSUtil method `setActiveClusterSuffix` will log,
but only at debug level. Please add some meaningful log message here too.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on PR #7857: URL: https://github.com/apache/hbase/pull/7857#issuecomment-4024724190 > This is how it looks like with this patch: > > ``` > 2026-03-09T15:02:52,121 INFO [main] hbase.TableName: Read Replica Cluster suffix value: > 2026-03-09T15:02:53,260 INFO [master/hbase-docker:16000:becomeActiveMaster] master.MasterFileSystem: Read Replica Cluster: This is the active cluster on this storage location with cluster id: 0115d976-1306-44c7-b8b6-f2644f8867c9: > ``` > > I suggest to use "" value in the log messages if the suffix is empty, because log messages currently are very confusing to me. > Will do the changes accordingly. > Second, what does this log message mean? > > ``` > 2026-03-09T15:02:53,259 WARN [master/hbase-docker:16000:becomeActiveMaster] util.FSUtils: Read Replica Cluster id file file:/data-store/hbase/active.cluster.suffix.id is empty > ``` > > What's empty? Why is it empty? I restarted the active cluster and the active cluster suffix file was present with the cluster id + blank suffix value: `0115d976-1306-44c7-b8b6-f2644f8867c9:` @anmolnar I am not much familiar with the nitty-gritty of this file based implementation as some other engineer worked on this but IIRC this may means that active.cluster.id.file may have been created without any data in it. I am not sure in which scenario this will happen. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
sharmaar12 commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2899496433
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
@@ -405,14 +405,14 @@ private void negotiateActiveClusterSuffixFile(long wait)
throws IOException {
this.activeClusterSuffix = acs;
} else {
// throw error
- LOG.info("rootdir {} : Active Cluster File Suffix {} ", rootdir,
acs);
+ LOG.info("rootdir {} : Read replica active cluster file suffix {} ",
rootdir, acs);
throw new IOException("Cannot start master, because another cluster
is running in active "
-+ "(read-write) mode on this storage location. Active Cluster Id:
{} " + acs
-+ " This cluster Id: " + getClusterId());
++ "(read-write) mode on this storage location. Active Cluster Id:
" + acs
++ ", This cluster Id: " + getClusterId());
Review Comment:
Updated the code accordingly. Sorry missed that part.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29958 Improve log messages [hbase]
anmolnar commented on code in PR #7857:
URL: https://github.com/apache/hbase/pull/7857#discussion_r2892743882
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
@@ -405,14 +405,14 @@ private void negotiateActiveClusterSuffixFile(long wait)
throws IOException {
this.activeClusterSuffix = acs;
} else {
// throw error
- LOG.info("rootdir {} : Active Cluster File Suffix {} ", rootdir,
acs);
+ LOG.info("rootdir {} : Read replica active cluster file suffix {} ",
rootdir, acs);
throw new IOException("Cannot start master, because another cluster
is running in active "
-+ "(read-write) mode on this storage location. Active Cluster Id:
{} " + acs
-+ " This cluster Id: " + getClusterId());
++ "(read-write) mode on this storage location. Active Cluster Id:
" + acs
++ ", This cluster Id: " + getClusterId());
Review Comment:
`getClusterId()` alone is not enough. We need to log the cluster id + suffix
like this:
```
dcf9e176-8437-49a9-9e91-b221b94e8fe3:
```
or
```
dcf9e176-8437-49a9-9e91-b221b94e8fe3:replica1
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
