Skip to content

Conversation

@srkukarni
Copy link
Contributor

Motivation

We go over all the declared fields in LocalRunner class to generate the command for localrunner. However one of the items that getDeclartedFields returns is the this$0 which should be ignored.

Modifications

Describe the modifications you've done.

Result

After your change, what will change.

@srkukarni srkukarni requested review from jerrypeng and sijie November 9, 2018 19:40
@srkukarni srkukarni self-assigned this Nov 9, 2018
@srkukarni srkukarni added this to the 2.3.0 milestone Nov 9, 2018
@srkukarni
Copy link
Contributor Author

rerun integration tests
rerun java8 tests

localRunArgs.add(new Gson().toJson(sinkConfig));
for (Field field : this.getClass().getDeclaredFields()) {
if (field.getName().startsWith("DEPRECATED")) continue;
if(field.getName().startsWith("this$0")) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since all anonymous classes and lambdas defined within a class will contain $N or $lambdaN, we should ignore everything with a $

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

for (Field field : this.getClass().getDeclaredFields()) {
if (field.getName().startsWith("DEPRECATED")) continue;
if(field.getName().startsWith("this$0")) continue;
if(field.getName().startsWith("$")) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be contains() instead of startsWith(), no? Anything with $ was auto created by compiler anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I have consolidated the logic

@srkukarni
Copy link
Contributor Author

rerun integration tests

@srkukarni srkukarni merged commit e8cce5c into apache:master Nov 10, 2018
@srkukarni srkukarni deleted the fix_localrun branch November 10, 2018 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants