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.

Sign up for GitHub

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

Jump to bottom

Converted the exception to an error message #5531

Merged
merged 7 commits into from
Jan 13, 2023
Merged

Converted the exception to an error message #5531

merged 7 commits into from
Jan 13, 2023

Conversation

mattjdnv
Copy link
Contributor

@mattjdnv mattjdnv commented Dec 13, 2022 •
edited by bmarchant
Loading

As the title says.
Now, unknown settings objects give an error instead of killing the job

Closes #5530

@mattjdnv mattjdnv requested a review from bmarchant December 13, 2022 21:12
@mattjdnv mattjdnv self-assigned this Dec 13, 2022
bmarchant
bmarchant reviewed Dec 14, 2022
View reviewed changes
hoot-core/src/main/cpp/hoot/core/util/Settings.cpp Outdated
Comment on lines 639 to 642
{
LOG_ERROR("Skipping unknown settings option: (" + optionName + ")");
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great!

@bmarchant
Copy link
Contributor

I have one question, are there other places in this file that throw exceptions that maybe need to be handled more gracefully?

This is from the same file on line 143. It is loading JSON config files while the other is command line arguments.

      //  Throw an exception for unrecognized keys
      if (!_s->hasKey(optionName))
        throw IllegalArgumentException("Unknown JSON setting: (" + optionName + ")");

@mattjdnv
Copy link
Contributor Author

Good point. I'll change that one as well.

Slightly different topic. I keep seeing this error in Jenkins but I can't reproduce it locally in Docker or in Vagrant. Any ideas?

<snip>
(0) ./home/vagrant/hoot/test-files/cases/serial/reference/unifying/railway/railway-5014-2 - 0.117681
(0) ./home/vagrant/hoot/test-files/cases/serial/reference/unifying/railway/railway-5044-1 - 2.34946
terminateHandler caught unhandled exception. what(): std::bad_alloc
stack trace:
  hoot::SignalCatcher::terminateHandler() +0x27
  /usr/lib64/libstdc++.so.6 : ()+0x5ea06
  /usr/lib64/libstdc++.so.6 : ()+0x5ea33
<snip>

@bmarchant bmarchant merged commit b150146 into master Jan 13, 2023
@bmarchant bmarchant deleted the 5530_errors branch January 13, 2023 22:20
jordanmurray35 pushed a commit that referenced this pull request Feb 1, 2023
@mattjdnv @bmarchant @jordanmurray35
Converted the exception to an error message (#5531)
3ed7ad8 
* Converted the exception to an error message

refs: #5530

* Converted a JSON settings file exception to an error.

refs: #5530

* Fix infinite loop when bad option is encountered

* Copyright :shakes-fist:

* Capture the log output to a string if desired, compare that in the Settings unit tests.

* Copyright

Co-authored-by: Ben Marchant <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmarchant bmarchant bmarchant left review comments

Assignees

@mattjdnv mattjdnv

Labels
Category: Core Priority: Medium Status: Ready for Test Type: Feature Type: Maintenance Type: Task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

More graceful error handling
2 participants
@mattjdnv @bmarchant