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

(CDAP-9250) Using reflection to find out the number of arguments getOperationStat… #8490

Merged

Conversation

Nishith
Copy link
Contributor

@Nishith Nishith commented Apr 5, 2017

…us takes and calling it accordingly.

int i;
// Find the getOperationStatus method so that we can check the number of arguments it expects.
Method[] methods = cliService.getClass().getMethods();
for (i = 0; i < methods.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little more concise and safe:

    OperationStatus operationStatus;
    Method getOperationStatus = null;
    for (Method method : getCliService().getClass().getMethods()) {
      if ("getOperationStatus".equals(method.getName())) {
        getOperationStatus = method;
      }
    }
    if (getOperationStatus == null) {
      // throw exception 
    }

Also, you can move this part to the start of the service so it only needs to be done once.

Copy link
Contributor

@anew anew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good now, only minor comments from me.

@@ -53,6 +56,7 @@
*/
public class Hive14ExploreService extends BaseHiveExploreService {

private Method getOperationStatus;
@Inject
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a newline?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and a comment what this is for?

}
}
if (getOperationStatus == null) {
throw new RuntimeException("Failed to get the status of the Hive operation. Unable to find getOperationStatus " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the first sentence. You are not actually getting a status yet :)

@Nishith Nishith force-pushed the bugfix_release/CDAP-9250-Change-getOperationStatus branch from 9a754e2 to f17e260 Compare April 6, 2017 00:27
@Nishith Nishith changed the title Using reflection to find out the number of arguments getOperationStat… (CDAP-9250) Using reflection to find out the number of arguments getOperationStat… Apr 6, 2017
@Nishith
Copy link
Contributor Author

Nishith commented Apr 6, 2017

Copy link
Contributor

@anew anew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@prinam prinam added the 4.1 label Apr 6, 2017
@Nishith Nishith merged commit 00a06fc into release/4.1 Apr 6, 2017
@Nishith Nishith deleted the bugfix_release/CDAP-9250-Change-getOperationStatus branch April 6, 2017 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants