Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-18 Thread via GitHub 


FANNG1 merged PR #4511:
URL: https://github.com/apache/gravitino/pull/4511


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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-15 Thread via GitHub 


Naresh-kumar-Thodupunoori commented on code in PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#discussion_r1718475894


##
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestFetchFileUtils.java:
##
@@ -22,13 +22,14 @@
 import java.io.File;
 import java.io.IOException;
 import org.apache.hadoop.conf.Configuration;
-import org.apache.log4j.Logger;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class TestFetchFileUtils {
 
-  private static final Logger logger = 
Logger.getLogger(TestFetchFileUtils.class);
+  Logger LOG = LoggerFactory.getLogger(TestFetchFileUtils.class);

Review Comment:
   did the needful!



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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-15 Thread via GitHub 


FANNG1 commented on code in PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#discussion_r1718444602


##
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestFetchFileUtils.java:
##
@@ -22,13 +22,14 @@
 import java.io.File;
 import java.io.IOException;
 import org.apache.hadoop.conf.Configuration;
-import org.apache.log4j.Logger;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class TestFetchFileUtils {
 
-  private static final Logger logger = 
Logger.getLogger(TestFetchFileUtils.class);
+  Logger LOG = LoggerFactory.getLogger(TestFetchFileUtils.class);

Review Comment:
   please keep `private static final `



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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-15 Thread via GitHub 


FANNG1 commented on PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#issuecomment-2291069157

   LGTM except one comment


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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-15 Thread via GitHub 


FANNG1 commented on code in PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#discussion_r1718258112


##
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestFetchFileUtils.java:
##
@@ -20,15 +20,20 @@
 package org.apache.gravitino.catalog.hive;
 
 import java.io.File;
+import java.io.IOException;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.log4j.Logger;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
 public class TestFetchFileUtils {
 
+  private static final Logger logger = 
Logger.getLogger(TestFetchFileUtils.class);

Review Comment:
please use slf4j
   ```java
   Logger LOG = LoggerFactory.getLogger(xxx.class);
   ```



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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-15 Thread via GitHub 


Naresh-kumar-Thodupunoori commented on PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#issuecomment-2291012103

   @jerryshao could help me with this. I am unable to fetch the error in the 
build using gradle as it is working fine in my local.


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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-15 Thread via GitHub 


Naresh-kumar-Thodupunoori commented on PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#issuecomment-2290950521

   @jerryshao It is building fine in my local
   
![image](https://github.com/user-attachments/assets/e2bad588-ddf0-482a-be59-e9d084d01bfd)
   


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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-15 Thread via GitHub 


Naresh-kumar-Thodupunoori commented on PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#issuecomment-2290844864

   @jerryshao Did the required changes.! If need to change anything else ping 
me!
   


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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-15 Thread via GitHub 


Naresh-kumar-Thodupunoori commented on PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#issuecomment-2290812380

   Ok, and would like me to change anything else!?
   
   On Thu, 15 Aug, 2024, 12:56 pm Jerry Shao, ***@***.***> wrote:
   
   > ***@***. commented on this pull request.
   > --
   >
   > In
   > 
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestFetchFileUtils.java
   > :
   >
   > > -Assertions.assertTrue(destFile.exists());
   > +String fileUrl = "https://downloads.apache.org/hadoop/common/KEYS";;
   > +Configuration conf = new Configuration();
   > +
   > +boolean success = false;
   > +int attempts = 0;
   > +
   > +while (!success && attempts < MAX_RETRIES) {
   > +  try {
   > +FetchFileUtils.fetchFileFromUri(fileUrl, destFile, 10, conf);
   > +success = true;
   > +  } catch (IOException e) {
   > +attempts++;
   > +if (attempts < MAX_RETRIES) {
   > +  System.out.println(
   > +  "Attempt " + attempts + " failed. Retrying in " + 
RETRY_DELAY_MS + "ms.");
   >
   > We don't allow to use println, please use log4j or remove this line.
   >
   > —
   > Reply to this email directly, view it on GitHub
   > 
,
   > or unsubscribe
   > 

   > .
   > You are receiving this because you authored the thread.Message ID:
   > ***@***.***>
   >
   


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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-15 Thread via GitHub 


jerryshao commented on code in PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#discussion_r1718063564


##
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestFetchFileUtils.java:
##
@@ -43,9 +46,29 @@ public void testLinkLocalFile() throws Exception {
   @Test
   public void testDownloadFromHTTP() throws Exception {
 File destFile = new File("dest");
-FetchFileUtils.fetchFileFromUri(
-"https://downloads.apache.org/hadoop/common/KEYS";, destFile, 10, new 
Configuration());
-Assertions.assertTrue(destFile.exists());
+String fileUrl = "https://downloads.apache.org/hadoop/common/KEYS";;
+Configuration conf = new Configuration();
+
+boolean success = false;
+int attempts = 0;
+
+while (!success && attempts < MAX_RETRIES) {
+  try {
+FetchFileUtils.fetchFileFromUri(fileUrl, destFile, 10, conf);
+success = true;
+  } catch (IOException e) {
+attempts++;
+if (attempts < MAX_RETRIES) {
+  System.out.println(
+  "Attempt " + attempts + " failed. Retrying in " + RETRY_DELAY_MS 
+ "ms.");

Review Comment:
   We don't allow to use `println`, please use log4j or remove this line.



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

Re: [PR] [#4483] add error retry to TestFetchFileUtils [gravitino]

2024-08-14 Thread via GitHub 


Naresh-kumar-Thodupunoori commented on PR #4511:
URL: https://github.com/apache/gravitino/pull/4511#issuecomment-2288115302

   @FANNG1 can you review my PR


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