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

HAWQ-1480 - Added feature for packing a core file #1251

Closed
wants to merge 2 commits into from

Conversation

outofmem0ry
Copy link
Contributor

Currently there is no way to packing a core file with its context – executable, application and system shared libraries in hawq. This information can be later unpacked on another system and helps in debugging. It is a useful feature to quickly gather all the data needed from a crash/core generated on the system to analyze it later.
Another open source project, greenplum, uses a script https://github.com/greenplum-db/gpdb/blob/master/gpMgmt/sbin/packcore to collect this information. Merging this into hawq.

# specific language governing permissions and limitations
# under the License.

# Copyright Pivotal 2014
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't suggest add "Copyright Pivotal 2014" here, since hawq is under Apache license.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@linwen It was in the original code repo I merged it from. Wanted to keep it there for reference/credits. Something similar to what is done here- incubator-hawq/tools/sbin/hawqstandbywatch.py

Let me know if it is feasible removing copyright from merged code. I can update my branch.

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 is better to remove it, also the ""Copyright Pivotal" in hawqstandbywatch.py. How do you think of this Roman? @rvs Thanks!

Copy link

Choose a reason for hiding this comment

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

We need to remove Pivotal copyright since we are contributing this code in Apache and doing so under Apache license.


def _getLibraryListWithLDD(binary):
# We manually seed this with a few libraries that are missed
# This may not be needed for all proces, but will round out the
Copy link
Contributor

Choose a reason for hiding this comment

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

"proces" is misspelled.

u = '''%prog [options] core_file
This will create an archive with the core file and all required
libraries for analysis. The preference is to use GDB so that we can
resolve dependancies for extensions.'''
Copy link
Contributor

Choose a reason for hiding this comment

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

"dependancies" is misspelled.

@edespino
Copy link
Contributor

edespino commented Jun 7, 2017

Aside from the two misspellings identified and the pending determination to keep or remove the "Copyright Pivotal 2014" string, this PR looks good. I have run the packcore against a HAWQ instance and it performed as expected. Will we need to add documentation for this utility?

@edespino
Copy link
Contributor

edespino commented Jun 7, 2017

Additionally, is a test being added to validate the functionality of packcore?

@linwen
Copy link
Contributor

linwen commented Jun 7, 2017

Ed, I agree with you on adding documentation for this utility. I have some concern on adding a test case, is it a little bit strange to have a core dump file in source repository(maybe two, OSX and Linux)? Or write a test case which can trigger a core dump, then run this script.

@outofmem0ry
Copy link
Contributor Author

@edespino @vVineet @linwen - Thank you for the reviews and comments. I have incorporated the changes and removed the copyright for now.

@linwen What kind of documentation do you have in mind? Here are some options -

  • A separate markdown explaining the use case and how to, committed to this repository
  • Use case and how to use packcore, committed under - incubator-hawq-docs/markdown/admin/

Additional suggestions are welcomed.

@edespino
Copy link
Contributor

edespino commented Jun 7, 2017

@outofmem0ry - Thanks for addressing the issues. FYI: It is helpful to push the changes to your repo:branch (outofmem0ry:feature-packcore) that this PR is based on so we can see that the issues were indeed addressed. The PR will automatically get updated when you push to your dev branch. Thanks, -=e

@edespino
Copy link
Contributor

edespino commented Jun 7, 2017

@outofmem0ry - It might be helpful to check with David Yozie (@dyozie) to see if we can get this into the public HAWQ user/admin documentation.

@dyozie
Copy link

dyozie commented Jun 8, 2017

I doesn't look like Greenplum documents packcore either.

@edespino
Copy link
Contributor

edespino commented Jun 8, 2017

I'm thinking from the perspective of an Apache developer. They shouldn't have to go to external corporate documentation sets or knowledge bases (https://discuss.pivotal.io/hc/en-us/articles/201081408-How-to-collect-core-files-for-analysis) to get useful information about a utility (packcore) included with the HAWQ distribution. As it stands, the utility (packcore) will be automatically bundled with all future HAWQ releases.

@outofmem0ry
Copy link
Contributor Author

@dyozie correct, it is not documented in greenplum.
@edespino - Totally agree with your point. Keeping that in mind, earlier, I suggested documenting it under incubator-hawq-docs/markdown/admin/ as this tool is more in an administrative capacity.

Let me know if that sounds good and I can start with the documentation.

@edespino
Copy link
Contributor

edespino commented Jun 8, 2017

@dyozie - is it easy for @outofmem0ry to add docs for this utility (packcore) in incubator-hawq-docs/markdown/admin/. Can you provide some guidance? It will be helpful to share this with the dev community for future reference.

@dyozie
Copy link

dyozie commented Jun 8, 2017 via email

@outofmem0ry
Copy link
Contributor Author

@dyozie @edespino - submitted documentation under incubator-hawq-docs pull Request #123.

@linwen
Copy link
Contributor

linwen commented Jun 12, 2017

LGTM

@edespino
Copy link
Contributor

This PR (sans documentation which is included in PR #123) LGTM.

@linwen
Copy link
Contributor

linwen commented Jun 13, 2017

merged, this pr can be closed now. Thanks!

@linwen
Copy link
Contributor

linwen commented Jun 14, 2017

Shubham, this PR has been merged into master. Would you please close it? Thanks!

@outofmem0ry
Copy link
Contributor Author

Thank you for the merge @linwen. Marking it closed.

@outofmem0ry outofmem0ry deleted the feature-packcore branch July 16, 2017 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants