HBASE-29958 Improve log messages#7922
Conversation
anmolnar
left a comment
anmolnar
left a comment
There was a problem hiding this comment.
lgtm. A quick question.
...server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java
Show resolved
Hide resolved
sharmaar12
force-pushed
the
log-message_rebase
branch
from
5b78ce6 to
3717463
Compare
anmolnar
force-pushed
the
HBASE-29081_rebased
branch
from
520b89a to
5d47ec9
Compare
sharmaar12
force-pushed
the
log-message_rebase
branch
from
97459b4 to
0b7c31f
Compare
sharmaar12
force-pushed
the
log-message_rebase
branch
from
0b7c31f to
9bf49e7
Compare
…s.java Co-authored-by: Kota-SH <shanmukhaharipriya@gmail.com>
…s.java Co-authored-by: Kota-SH <shanmukhaharipriya@gmail.com>
…erFileSystem.java Co-authored-by: Kota-SH <shanmukhaharipriya@gmail.com>
sharmaar12
force-pushed
the
log-message_rebase
branch
from
9bf49e7 to
a6828ad
Compare
| } | ||
| } catch (IOException e) { | ||
| // We still update the flag, but log that the operation failed. | ||
| LOG.error("Failed to perform file operation for read-only switch. " |
There was a problem hiding this 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.
There was a problem hiding this 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.
| LOG.debug("Active cluster file already exists at: {}. No need to create it again.", | ||
| activeClusterFile); |
There was a problem hiding this 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.
|
I've fun the failing unit test 3 times locally without an error, so I go ahead and merge this patch. |
2 participants
No description provided.