devmadhuu commented on code in PR #9627:
URL: https://github.com/apache/ozone/pull/9627#discussion_r2686929017


##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconServer.java:
##########
@@ -104,9 +108,23 @@ public Void call() throws Exception {
             ReconServer.class, originalArgs, LOG, configuration);
     ConfigurationProvider.setConfiguration(configuration);
 
+    String reconStarterUser = 
UserGroupInformation.getCurrentUser().getShortUserName();
+    Collection<String> adminUsers =
+        OzoneAdmins.getOzoneAdminsFromConfig(configuration, reconStarterUser);

Review Comment:
   Instead of calling Admins and AdminsGroups method separately here, can we 
use 
[this](https://github.com/apache/ozone/blob/ae799bd73c2bd10faa521a5f365cffc7556675ec/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java#L78)
 method directly and then add recon admins and recon admin groups. this will 
minimise the code lines.



##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconServer.java:
##########
@@ -427,4 +445,32 @@ public ReconTaskController getReconTaskController() {
   ReconHttpServer getHttpServer() {
     return httpServer;
   }
+
+  /**
+   * Get the collection of Recon admin usernames.
+   *
+   * @return Collection of admin usernames
+   */
+  public Collection<String> getReconAdminUsernames() {
+    return reconAdmins.getAdminUsernames();
+  }
+
+  /**
+   * Get the collection of Recon admin groups.
+   *
+   * @return Collection of admin groups
+   */
+  public Collection<String> getReconAdminGroups() {

Review Comment:
   Any specific usecase or need to define these admin and admin group getter 
methods ?



##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconControllerModule.java:
##########
@@ -92,8 +92,15 @@ public class ReconControllerModule extends AbstractModule {
   private static final Logger LOG =
       LoggerFactory.getLogger(ReconControllerModule.class);
 
+  private final ReconServer reconServer;
+
+  public ReconControllerModule(ReconServer reconServer) {
+    this.reconServer = reconServer;
+  }
+
   @Override
   protected void configure() {
+    bind(ReconServer.class).toInstance(reconServer);

Review Comment:
   why we need `reconServer` instance to be passes explicitly to 
`ReconControllerModule` ? can we not use Singleton ?



-- 
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]

Reply via email to