errose28 commented on code in PR #10236:
URL: https://github.com/apache/ozone/pull/10236#discussion_r3319785788
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/upgrade/TestOMUpgradeFinalizeService.java:
##########
Review Comment:
Since request processing is mocked in this test, we should have a unit test
for OMFinalizeUpgradeRequest/Response classes as well, similar to other OM
requests. Looks like we didn't have those previously but they would be good to
add.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshot.java:
##########
@@ -234,6 +234,9 @@ private void init() throws Exception {
// Enable filesystem snapshot feature for the test regardless of the
default
conf.setBoolean(OMConfigKeys.OZONE_FILESYSTEM_SNAPSHOT_ENABLED_KEY, true);
conf.setInt(OMStorage.TESTING_INIT_APPARENT_VERSION_KEY,
OMLayoutFeature.BUCKET_LAYOUT_SUPPORT.layoutVersion());
+ conf.setInt(SCMStorageConfig.TESTING_INIT_LAYOUT_VERSION_KEY,
+ HDDSLayoutFeature.HADOOP_PRC_PORTS_IN_DATANODEDETAILS.layoutVersion());
Review Comment:
In the updated flow SCM will finalize when instructed by OM from a user
command, and then OM will follow. This prevents OM from finalizing before the
user command just because a release had no HDDS version increase. Therefore the
original change that was commented on was removed.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -297,7 +297,7 @@ public void testInstallSnapshot(@TempDir Path tempDir)
throws Exception {
String toMatch = String.format(
"op=DB_CHECKPOINT_INSTALL
{\"leaderId\":\"%s\",\"term\":\"%d\",\"lastAppliedIndex\":\"%d\"}",
leaderOMNodeId, leaderOMSnapshotTermIndex, followerOMLastAppliedIndex);
- assertTrue(AuditLogTestUtils.auditLogContains(toMatch));
+ assertTrue(AuditLogTestUtils.systemAuditLogContains(toMatch));
Review Comment:
This change is because we split the audit log configuration so that system
audit logs go to their own logger, which is an improvement since the test can
now distinguish between audit and system audit log messages and not accept one
when it should have only accepted the other.
##########
hadoop-ozone/integration-test/src/test/resources/auditlog.properties:
##########
@@ -52,27 +52,34 @@ filter.write.onMismatch = NEUTRAL
# TRACE (least specific, a lot of data)
# ALL (least specific, all data)
-appenders = console, audit
+appenders = console, audit, systemaudit
appender.console.type = Console
appender.console.name = STDOUT
appender.console.layout.type = PatternLayout
-appender.console.layout.pattern = %d{DEFAULT} | %-5level | %c{1} | %msg |
%throwable{3} %n
+appender.console.layout.pattern = %-5level | %c{1} | %msg%n
Review Comment:
This is removing the exception and timestamp from being logged to the
console. IMO we should restore this to what it was before since it is easer to
trace log messages in a test env from the console than opening dedicated files.
`additivity` was also set to `false` for the audit logs which means they won't
go to the console anymore. I think we should remove that so all log messages
are searchable in one place.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -1900,6 +1908,10 @@ public void start() throws IOException {
bootstrap(omNodeDetails);
}
+ if (omUpgradeFinalizeService != null) {
Review Comment:
Ok this works. We could alternatively start the thread when the command is
received (in the next PR) if it is not too difficult to connect the pieces.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
