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

Add option to allow Lumberjack to own communicator #1513

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

gberg617
Copy link
Contributor

@gberg617 gberg617 commented Mar 5, 2025

Summary

  • This PR is a feature
  • This PR adds the ability for Lumberjack to directly own a Communicator object. When this case is true, Lumberjack will call delete on the communicator object when finalize() is called.

@rhornung67
Copy link
Member

setCommunicator() method needs to be added. Also, should initialize() check to make sure it's not being called more than it should? @white238 any additional comments?

Copy link
Contributor

@gunney1 gunney1 left a comment

Choose a reason for hiding this comment

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

Looks okay to me pending the comment of @white238 .

@gberg617
Copy link
Contributor Author

To Address comments from @white238, the following were added to this PR:

  1. Methods in Lumberjack to get and swap communicators, as well as a getter for the isCommunicatorOwned flag.
  2. isInitialized flag added to enforce initialize() to be called only once per Lumberjack instance.
  3. Unit tests to check correct behavior for swapping owned and non-owned communicators in Lumberjack
  4. Documentation
  5. Release notes

@bmhan12
Copy link
Contributor

bmhan12 commented Mar 12, 2025

  1. Documentation

👍
Updated documentation for this PR can be found here: https://axom.readthedocs.io/en/feature-bergel1-lumberjack_set_communicator/axom/lumberjack/docs/sphinx/lumberjack_class.html

Copy link
Member

@rhornung67 rhornung67 left a comment

Choose a reason for hiding this comment

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

Please add setCommunicator(). Actually, if setCommunicator() returned existing communicator, then swapCommunicator() would not be needed.

@gberg617
Copy link
Contributor Author

Please add setCommunicator(). Actually, if setCommunicator() returned existing communicator, then swapCommunicator() would not be needed.

@rhornung67 I changed the name of swapCommunicator to setCommunicator. It really does a set and not a swap, so the new naming makes more sense. A swap would probably involve passing in another Lumberjack object, and swapping its communicator with the current one (or something along these lines). I don't currently implement this type of functionality. Would it be useful at all to add it in? Doing a swap may be somewhat challenging because we would have to specially handle cases where Lumberjack owns the communicator.

@rhornung67
Copy link
Member

@gberg617 my preference is to just go with setCommunicator for now. If we need a swap method at some point, we can deal with it then.

Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

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

Thanks @gberg617

Comment on lines 199 to 207
void Lumberjack::setCommunicator(Communicator* communicator)
{
if(m_isCommunicatorOwned && m_communicator != nullptr)
{
m_communicator->finalize();
delete m_communicator;
}
m_communicator = communicator;
}
Copy link
Member

Choose a reason for hiding this comment

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

Should this function allow the user to indicate if the new communicator is owned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a bool flag to allow the user to set the ownership of the communicator passed in. I am assuming that the user has to set the ownership flag (i.e. it's not an optional parameter). Let me know if there are any issues with this.

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.

6 participants