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

[ROCKETMQ-42] Output is not friendly, when Main failed. #35

Closed
wants to merge 8 commits into from

Conversation

wu-sheng
Copy link
Member

@asfgit asfgit closed this in 0661ac8 Jan 20, 2017
@wu-sheng
Copy link
Member Author

Whether the pull request be merged or not, the project team should close it without anything.

Using a commit to close, only comment on "won't fix", seem not regular for outside contributors. More communications should stay on GitHub, rather than your inside team, as least have a discussion, and then have a conclusion. In Apache Software Foundation, benevolent dictator is not welcome, I think you should pay attention to this.

This is only a suggestion, but I think this is important. btw @vongosling @WillemJiang

@zhouxinyu
Copy link
Member

Hi @wu-sheng , I am sorry for this rash close~

Actually, as @shroman mentioned in this issue, we also think avoid using e.printStackTrace() or system.out is a better way, and there are some discussions in this thread.

Finally, thanks for this suggestion, we will have more discussions in GitHub issue before closing a PR.

@wu-sheng
Copy link
Member Author

I have known about @shroman 's issue. And I am sure he is right about that log is better than e.printStackTrace() or system.out. And here, there is a special scene.

Let's focus on FiltersrvStartup.java's change logs. This system.out is unavoidable.

  • try-catch block starts at line 74
  • Field log is initialized at line 128

If codes occur exception between these two lines, certainly be catched, and trigger NullPointException in catch.

Other changes like this one, this is why I only change these classes, as so many System.outs are used in project.

You see, this is also why we need discuss before merge or close. 😊

@wu-sheng
Copy link
Member Author

Maybe, we should reopen this pull request, and discuss more details.

@wu-sheng
Copy link
Member Author

@vongosling, your idea?

Copy link

@chandresh-pancholi chandresh-pancholi left a comment

Choose a reason for hiding this comment

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

Why aren't you logging it rather than doing SOP.

@wu-sheng
Copy link
Member Author

I have already explain it before. Generally speaking, log is not initialized.

lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
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

3 participants