Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MLSQLRest returns more informative response for different types of errors #1739

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

chncaesar
Copy link
Contributor

What changes were proposed in this pull request?

  1. Fixed RestUtils.executeWithRetrying executes one more time than expected.
  2. MLSQLRest load function refactoring and 4 new test cases.
  3. More informative response

RestUtils.executeWithRetrying executes one more time than expected.

In the old implementation, function executes at least two times. It's fixed now, and is covered by test case in RestUtilsTest

 def executeWithRetrying[T](maxTries: Int)(function: => T, checker: T => Boolean, failed: T => Unit): T = {
   // Run the function for the first time
    var result: T = function
    if (checker(result)) {
      return result
    }

    breakable {
      for (i <- 0 until maxTries) {
       // Run again even if maxTries == 1
        result = function
        if (checker(result)) {
          break
        }
        if (i == maxTries - 1) {
          failed(result)
        }
      }
    }
    result
  }

MLSQLRest load function refactoring

The logic to call http endpoints with retrying is repated 3 times with small differences. Now , we encapsulate it in
_httpWithRetrying. For pagnation, use do while loop to avoid reapted code.

Since it's a big change, 4 new caes are added. And unit test covers

  • Single page http rest
  • Three pagination strategies: auto-increment, default and offset st.
  • Stop strategy : bad status-code, config.page.limit, sizeZero, equals

More informative response

Now, MLSQLRest returns

  • 405 for unsupported http method
  • 415 for unsupported content-type
  • 0 for Exception such as UnknownHost.
  • 4xx for client side errors
  • 5xx for server side errors

How was this patch tested?

MLSQLRest test result
image

RestUtils test result
image

Are there and DOC need to update?

  • Changes does not impact current documents.

Spark Core Compatibility

@chncaesar
Copy link
Contributor Author

Issue #1738

@allwefantasy allwefantasy self-requested a review April 6, 2022 09:57
…pected issue.

2. MLSQLRest load function refactoring and 2 new unit test cases.
@allwefantasy allwefantasy merged commit 642af2d into byzer-org:master Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants