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

[HOTFIX] remove this useless assignment #3017

Closed
wants to merge 1 commit into from
Closed

[HOTFIX] remove this useless assignment #3017

wants to merge 1 commit into from

Conversation

lamberken
Copy link
Member

@lamberken lamberken commented Dec 23, 2018

Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:

  • Any interfaces changed? NO

  • Any backward compatibility impacted? NO

  • Document update required? NO

  • Testing done YES
    Please provide details on
    - Whether new unit test cases have been added or why no new tests are required?
    - How it is tested? Please attach test report.
    - Is it a performance related change? Please attach the performance test report.
    - Any additional information to help reviewers in testing this change.

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA. OK

@lamberken
Copy link
Member Author

status was already assigned value false

@CarbonDataQA
Copy link

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1910/

@CarbonDataQA
Copy link

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10164/

@CarbonDataQA
Copy link

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2121/

@@ -112,7 +112,7 @@ public LocalFileLock(String lockFileLocation, String lockFile) {
status = true;
}
} catch (IOException e) {
status = false;
// status = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it print error message for IOException ?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is no need

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we print a warning message, and remove this assignment
@ravipesala @QiangCai

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest we print a warning message, and remove this assignment
@ravipesala @QiangCai

ok, I saw some exception just catch, not print message. so I think it's no need here before, like HdfsFileLock

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think HdfsFileLock has similar problem. Actually there is a PR #2878 trying to fix all exception code style problem, by @kevinjmh . I suggest we can have it handled in that PR. Then for this PR, I think you can remove the assignment directly, same for HdfsFileLock

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think HdfsFileLock has similar problem. Actually there is a PR #2878 trying to fix all exception code style problem, by @kevinjmh . I suggest we can have it handled in that PR. Then for this PR, I think you can remove the assignment directly, same for HdfsFileLock

ok, I see

Copy link
Member

Choose a reason for hiding this comment

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

Please review PR #2878 at your convenience. Changes follows rules in the table in description

@lamberken
Copy link
Member Author

lamberken commented Dec 26, 2018

hi, @jackylk, I think this may be part of code style as we talk about in wechat group. So need someone control the progress of code style

@CarbonDataQA
Copy link

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1957/

@lamberken
Copy link
Member Author

@jackylk , to avoid conflict, I should close this PR and create a new PR when #2878 finished.

@lamberken lamberken closed this Dec 27, 2018
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.

None yet

5 participants