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

Log Level Review #28

Merged
merged 7 commits into from
Jun 27, 2023
Merged

Log Level Review #28

merged 7 commits into from
Jun 27, 2023

Conversation

dmccoystephenson
Copy link
Collaborator

Changes

Reviewed and adjusted the log levels for each log statement as necessary.

Replaced std::cout & std::cerr with the logger whenever possible, except for certain cases:

  • Error messages related to logger initialization.
  • A log statement in Tool.cpp.
  • A warning log statement if the logger is configured to not print to the console or a file.

Additional Changes

  • Added some files to .gitignore.
  • Modified the build_local.sh script to remove the 'build' directory before compilation.
  • Added the 'topic.Asn1DecoderInput' and 'topic.Asn1DecoderOutput' topics to the list of topics in docker-compose.yml.
  • Marked the files in the /kafka-test directory as deprecated.

Testing

  • Verified the program compiles and starts using build_local.sh and docker-compose.yml.
  • These changes have been deployed to our dev cluster and data has been verified to still be flowing.

@@ -32,6 +32,9 @@
# Cyber and Information Security Research (CISR) Group Oak Ridge National
# Laboratory
# 865-804-5161 (mobile)

# NOTE: THIS FILE IS DEPRECATED AS OF 5/17/2023 AND IS NOT RECOMMENDED FOR USE.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this deprecated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The files in the kafka-test directory are separate from the primary project, not referenced anywhere and have not been tested recently to my knowledge.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is added here.

add_subdirectory(kafka-test)

If we run the cmake command with higher cmake version 3 at the root folder, this cmake 2.6 will report cmake version is low.
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I stand corrected, the deprecation note has been removed.

build_local.sh Outdated
@@ -57,6 +57,7 @@ compileSpecAndBuildLibrary(){
buildACM(){
# Build the ACM
echo "${GREEN}Building ACM${NC}"
rm -r build
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check if build folder exist before remove it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This check has been added to the script via CDOT-CV#11.

@@ -32,6 +32,8 @@
* (https://github.com/edenhill/librdkafka)
*/

// NOTE: THIS FILE IS DEPRECATED AS OF 5/17/2023 AND IS NOT RECOMMENDED FOR USE.

Copy link
Contributor

Choose a reason for hiding this comment

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

The same as the above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The files in the kafka-test directory are separate from the primary project, not referenced anywhere and have not been tested recently to my knowledge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This deprecation note has been removed.

RdKafka::PartitionMetadata::ISRSIterator iis;
for (iis = ip->isrs()->begin(); iis != ip->isrs()->end() ; ++iis)
std::cout << (iis == ip->isrs()->begin() ? "":",") << *iis;
logger->info(" " + std::to_string(*iis));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this " " for all logger?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we need to include " " in all logging messages. In this for loop the data should be separated by something since it was being separated before.

…d-folder-exists-before-removing

Added check for build folder existence before deletion
@dmccoystephenson
Copy link
Collaborator Author

dmccoystephenson commented Jun 27, 2023

This is ready for re-review! @dan-du-car

@dan-du-car dan-du-car merged commit f3992a5 into usdot-jpo-ode:develop Jun 27, 2023
dan-du-car pushed a commit that referenced this pull request Jun 11, 2024
Write output from the ASN1_Codec::filetest function directly to stdout
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

4 participants