pareshddevalia commented on code in PR #730:
URL: https://github.com/apache/atlas/pull/730#discussion_r3893539576


##########
notification/src/main/java/org/apache/atlas/notification/rest/RestNotification.java:
##########
@@ -115,11 +114,10 @@ private AtlasClientV2 setupAtlasClientV2(Configuration 
configuration) throws Atl
             String[] atlasEndPoint = 
configuration.getStringArray(ATLAS_HOOK_REST_NOTIFICATION_ENDPOINT);
 
             if (isEndpointNotSpecified(atlasEndPoint)) {
-                atlasEndPoint = 
configuration.getStringArray(AtlasConstants.ATLAS_REST_ADDRESS_KEY);
-            }
-
-            if (isEndpointNotSpecified(atlasEndPoint)) {
-                atlasEndPoint = new String[] {DEFAULT_ATLAS_URL};
+                throw new AtlasException("atlas.hook.rest.notification.address 
must be configured when REST notification is enabled. "

Review Comment:
   can we update this throw new 
AtlasException(ATLAS_HOOK_REST_NOTIFICATION_ENDPOINT + " is required for 
rest-notification-webapp (e.g. "
                           + AtlasConstants.DEFAULT_REST_NOTIFICATION_ADDRESS + 
")");



##########
notification/src/test/java/org/apache/atlas/notification/RestNotificationTest.java:
##########
@@ -150,36 +152,48 @@ public void 
testRestNotificationEndpointPrefersHookRestAddress() throws Exceptio
         assertEquals(configuredEndpoints[0], 
"http://atlas-rest.example.com:41000/rest";);
     }
 
-    @Test
-    public void testRestNotificationEndpointFallsBackToAtlasRestAddress() 
throws Exception {
+    @Test(expectedExceptions = AtlasException.class)
+    public void testRestNotificationFailsFastWhenHookAddressNotConfigured() 
throws Exception {
         Configuration localConf = new BaseConfiguration();
-        localConf.setProperty("atlas.rest.address", 
"http://atlas-main.example.com:21000";);
         localConf.setProperty("atlas.rest.basic.auth.username", "admin");
         localConf.setProperty("atlas.rest.basic.auth.password", "admin123");
 
-        RestNotification restNotification = new RestNotification(localConf);
-
-        String[] configuredEndpoints = 
getConfiguredBaseUrls(restNotification.atlasClientV2);
-
-        assertEquals(configuredEndpoints.length, 1);
-        assertEquals(configuredEndpoints[0], 
"http://atlas-main.example.com:21000";);
+        new RestNotification(localConf);
     }
 
-    @Test
-    public void 
testRestNotificationFallsBackToAtlasRestWhenNotificationAddressesAllBlank() 
throws Exception {
+    @Test(expectedExceptions = AtlasException.class)
+    public void testRestNotificationFailsFastWhenHookAddressesAllBlank() 
throws Exception {
         Configuration localConf = new BaseConfiguration();
         localConf.addProperty("atlas.hook.rest.notification.address", "");
         localConf.addProperty("atlas.hook.rest.notification.address", "   ");
         localConf.setProperty("atlas.rest.address", 
"http://atlas-main.example.com:21000";);
         localConf.setProperty("atlas.rest.basic.auth.username", "admin");
         localConf.setProperty("atlas.rest.basic.auth.password", "admin123");
 
-        RestNotification restNotification = new RestNotification(localConf);
+        new RestNotification(localConf);
+    }
 
-        String[] configuredEndpoints = 
getConfiguredBaseUrls(restNotification.atlasClientV2);
+    @Test(expectedExceptions = AtlasException.class)
+    public void 
testRestNotificationFailsFastWhenOnlyAtlasRestAddressConfigured() throws 
Exception {
+        Configuration localConf = new BaseConfiguration();
+        localConf.setProperty("atlas.rest.address", 
"http://atlas-main.example.com:21000";);
+        localConf.setProperty("atlas.rest.basic.auth.username", "admin");
+        localConf.setProperty("atlas.rest.basic.auth.password", "admin123");
 
-        assertEquals(configuredEndpoints.length, 1);
-        assertEquals(configuredEndpoints[0], 
"http://atlas-main.example.com:21000";);
+        new RestNotification(localConf);
+    }
+
+    @Test
+    public void testRestNotificationFailFastExceptionMessage() {
+        Configuration localConf = new BaseConfiguration();
+
+        try {
+            new RestNotification(localConf);
+            fail("Expected AtlasException when 
atlas.hook.rest.notification.address is not configured");
+        } catch (AtlasException e) {
+            
assertTrue(e.getMessage().contains("atlas.hook.rest.notification.address must 
be configured"));

Review Comment:
   instead of atlas.hook.rest.notification.address must be configured you can 
add atlas.hook.rest.notification.address is required



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

Reply via email to