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

Feature dtcenter/METplus-Internal#19 log user ID and dtcenter/METplus-Internal#21 signal handling #2160

Merged
merged 13 commits into from
Aug 3, 2022

Conversation

hsoh-u
Copy link
Collaborator

@hsoh-u hsoh-u commented May 12, 2022

Expected Differences

This is a prototype for the design review for login user ID and adding signal handlers at the centralized location. Note: only ascii2nc was modified to demonstrate the design.

  • Common implementation at main.h & main.cc

    • "int main(...)" method is implemented at basic/vx_log/main.cc
    • pre-processing
      • Log message with when, who, which MET apps
      • Install signal handlers
    • calls met_main()
    • post-processing
      • uninstall signal handlers
      • Log message with when, who, which MET apps
  • each MET tools (for example, ascii2nc)

    • "int main(...)" method should be renamed to ""int met_main(...)" method
    • "const char *get_tool_name()" should be implemented
  • exit() should be modified to call the post_processing or should create a custom signal

  • Do these changes introduce new tools, command line arguments, or configuration file options? [Yes or No]

    If yes, please describe:

  • Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [Yes or No]

    If yes, please describe:

Pull Request Testing

  • [] Describe testing already performed for these changes:

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]

  • Do these changes include sufficient testing updates? [No]

  • Will this PR result in changes to the test suite? [No]

    If yes, describe the new output and/or changes to the existing output:

  • Please complete this pull request review by [Fill in date].

Pull Request Checklist

See the METplus Workflow for details.

  • Review the source issue metadata (required labels, projects, and milestone).
  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: Reviewer(s)
    Select: Organization level software support Project or Repository level development cycle Project
    Select: Milestone as the version that will include these changes
  • After submitting the PR, select Linked issues with the original issue number.
  • After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
  • Close the linked issue and delete your feature or bugfix branch from GitHub.

@hsoh-u hsoh-u changed the title Feature me tplus internal 19 log user Feature METplus-Internal #19 log user and #21 signal handling May 12, 2022
@hsoh-u hsoh-u changed the title Feature METplus-Internal #19 log user and #21 signal handling Feature METplus-Internal #19 log user ID and #21 signal handling May 12, 2022
Copy link
Collaborator

@georgemccabe georgemccabe left a comment

Choose a reason for hiding this comment

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

These changes look like a good approach to adding the desired information to the run of each MET tool. The changes needed for each other tool would be minimal with this approach. I made a few suggested edits inline. There are a few commented lines of code that could be removed as well. I approve.


////////////////////////////////////////////////////////////////////////

void do_post_preocess(uid_t user_id, const char *tool_name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
void do_post_preocess(uid_t user_id, const char *tool_name) {
void do_post_process(uid_t user_id, const char *tool_name) {


int return_code = met_main(argc, argv);

do_post_preocess(user_id, tool_name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
do_post_preocess(user_id, tool_name);
do_post_process(user_id, tool_name);


void do_post_preocess(uid_t user_id, const char *tool_name) {
cout << " Do post-processing\n";
cout << " Done " << tool_name << " by user " << user_id << " at " << get_current_time() << "\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider changing "Done" to "Finished" or "Completed"

@jprestop jprestop removed their request for review May 12, 2022 21:44
@JohnHalleyGotway JohnHalleyGotway added this to the MET 11.0.0 milestone May 19, 2022

return return_code;

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

// Filename: main.cc
//
// Description:
// The main() should be inplemented here and the exisyting main() should be renamed to met_main.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// The main() should be inplemented here and the exisyting main() should be renamed to met_main.
// The main() should be implemented here and the existing main()
// should be renamed to met_main().
//
////////////////////////////////////////////////////////////////////////

Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

Howard, I get the general idea here, creating a wrapper for the existing main() function of each tool. And it seems like a promising approach. One issue I see is writing the timestamps to cout instead of via the logger. Is there a way to route the messages through mlog instead so that they can be save to the generated log file?

@hsoh-u hsoh-u requested a review from JohnHalleyGotway July 8, 2022 17:08
@hsoh-u
Copy link
Collaborator Author

hsoh-u commented Jul 8, 2022

Typos were corrected.
More code changes:

  • main() at main.cc
    • use mlog
    • calls set_new_handler(oom)
      • this causes link problems, so Makefile.am was modified
  • The source codes under src/tools were modified
    • main is renamed to met_main
    • get_tool_name() is added
    • set_new_handler(oom) is removed because it is called at the main.cc.
    • Note: src/basic/enum_to_string/enum_to_string was not updated (let me know this should be updated, too)

To do list:

  • move initalize() and process_command_line to main() at main.cc
    • not updated yet because this causes another link issues
  • log event (start/finished) to the centralized place

@hsoh-u hsoh-u requested a review from georgemccabe July 8, 2022 22:23
@JohnHalleyGotway JohnHalleyGotway requested review from georgemccabe and removed request for georgemccabe August 2, 2022 23:30
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

@hsoh-u there is a single main() function you missed updating:
src/tools/other/wwmca_tool/wwmca_regrid.cc:int main(int argc, char * argv [])

After updating that one, please proceed with the merge.
I see via GHA that the code still compiles and produces no unexpected diffs.
We should include this change in the beta2 release tomorrow so that we have plenty of time to test it out and make sure it doesn't cause any unintended compilation/runtime consequences.

I approve, subject to updating wwmca_regrid.cc.

Thanks for all your detailed work on this!

@hsoh-u hsoh-u merged commit 5c0804b into develop Aug 3, 2022
@JohnHalleyGotway JohnHalleyGotway changed the title Feature METplus-Internal #19 log user ID and #21 signal handling Feature dtcenter/METplus-Internal#19 log user ID and dtcenter/METplus-Internal#21 signal handling Aug 3, 2022
@hsoh-u hsoh-u deleted the feature_METplus-Internal_19_log_user_id branch August 11, 2022 15:20
@JohnHalleyGotway
Copy link
Collaborator

Note that this issue also resolved dtcenter/METplus-Internal#18 since it logs the beginning/ending time of each run.

@TaraJensen TaraJensen added the reporting: DTC AF METplus Air Force METplus Project label Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reporting: DTC AF METplus Air Force METplus Project
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants