Skip to content

Conversation

@yemreinci
Copy link
Contributor

@yemreinci yemreinci commented Nov 13, 2020

This PR includes some enhancements for the logging implementation:

  • Use the new client::version() API instead of the HAZELCAST_VERSION macro in the default formatter. Fixes Logging comment #682.
  • Add tests for logger_config.
  • Suppress MSVC C4151 warning in logger.h
  • Don't use localtime. It is not guaranteed to be thread-safe. Instead, use localtime_s or localtime_r depending on the platform.

@yemreinci yemreinci self-assigned this Nov 13, 2020
@yemreinci yemreinci added this to the 4.0 milestone Nov 13, 2020
}

namespace {

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just utilize utilities from boost and not have an extra method? did you check boost? e.g. https://stackoverflow.com/questions/2492775/get-local-time-with-boost The boost::posix_time::second_clock::local_time() uses localtime_r

Copy link
Collaborator

Choose a reason for hiding this comment

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

We decided to do this later before 4.0 release. An issue will be opened for this.

@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

Copy link
Collaborator

@ihsandemir ihsandemir left a comment

Choose a reason for hiding this comment

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

Once the issue is opened for localtime all is OK.

@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

@ihsandemir
Copy link
Collaborator

related to #692

@ihsandemir ihsandemir merged commit bc43312 into hazelcast:master Nov 16, 2020
@yemreinci yemreinci deleted the logger-enhancements branch January 21, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logging comment

3 participants