devmadhuu commented on code in PR #11246:
URL: https://github.com/apache/ozone/pull/11246#discussion_r4023223000
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java:
##########
@@ -147,7 +147,10 @@ public class OzoneContainer {
recoveringContainerScrubbingService;
private final GrpcTlsConfig tlsClientConfig;
private DiskBalancerService diskBalancerService;
+ private final Object initializationLock = new Object();
+ // Guarded by initializationLock.
private final AtomicReference<InitializingStatus> initializingStatus;
Review Comment:
Do we need this as an `AtomicReference` if ussing Object lock ?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java:
##########
Review Comment:
Right now we know some known issue in RATIS, but what if in future some
other regression in RATIS makes hang forever, should we decouple this by having
some timeout to avoid an infinite hold of `initializationLock` ?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java:
##########
@@ -547,24 +550,30 @@ public OnDemandContainerScanner getOnDemandScanner() {
* @throws IOException
*/
public void start(String clusterId) throws IOException {
- // If SCM HA is enabled, OzoneContainer#start() will be called multi-times
- // from VersionEndpointTask. The first call should do the initializing job,
- // the successive calls should wait until OzoneContainer is initialized.
- if (!initializingStatus.compareAndSet(
- InitializingStatus.UNINITIALIZED, InitializingStatus.INITIALIZING)) {
-
- // wait OzoneContainer to finish its initializing.
- while (initializingStatus.get() != InitializingStatus.INITIALIZED) {
- try {
- Thread.sleep(1);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
+ synchronized (initializationLock) {
+ // SCM endpoints share one initialization attempt, including its failure.
+ if (initializingStatus.get() == InitializingStatus.INITIALIZED) {
+ LOG.info("Ignore. OzoneContainer already started.");
+ return;
+ }
+ if (initializingStatus.get() == InitializingStatus.FAILED) {
+ throw new IOException("OzoneContainer initialization previously
failed", initializationFailure);
+ }
+
+ initializingStatus.set(InitializingStatus.INITIALIZING);
+ try {
+ initializeContainerServices(clusterId);
+ initializingStatus.set(InitializingStatus.INITIALIZED);
+ } catch (IOException | RuntimeException | Error ex) {
Review Comment:
Is it not too generic to handle all errors ? What if any OOM. It will not
let `InterruptedException` also to interrupt the thread.
--
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]
