[SDL-0117] Configurable time before shutdown #3740

VladSemenyuk · 2021-07-19T15:20:05Z

Implements #1941

This PR is not ready for review.

Risk

This PR makes no API changes.

Testing Plan

ATF scripts

Summary

Add Logger config settings with params for configurable time before shutdown.
Update SDL config.
Extend LoggerImpl class for shutdown configuration.
Extend MessageLoopThread, MessageQueque and ConditionlVariable classes with new wait mechanism.

CLA

I have signed the CLA

…hutdown. Update SDL config. Extend LoggerImpl class for shutdown configuration. Extend MessageLoopThread, MessageQueque and ConditionlVariable classes with new wait mechanism.

yang1070 · 2021-07-26T20:50:49Z

src/components/utils/src/logger/logger_impl.cc

+  if (use_message_loop_thread_ && loop_thread_) {
+    assert(settings_);
+
+    if (0 == flush_logs_time_point_.time_since_epoch().count()) {


why do we have such a condition? it seems impossible to me that count() is zero, thus impossible to execute next line

Default constructor, creates a time_point representing the Clock's epoch (i.e., time_since_epoch() is zero).

@VladSemenyuk in file on_exit_all_applications_notification.cc and function void OnExitAllApplicationsNotification::Run()
it calls SDL_INIT_FLUSH_LOGS_TIME_POINT(std::chrono::system_clock::now()); to set the value of flush_logs_time_point_ to non-zero, what's the condition that next line get executed? during the SDL running without receiving ExitAllApplicationsNotification?

Yes, ExitAllApplicationsNotification is not the only exit point for SDL.

@VladSemenyuk could you list what are other conditions or exit point to call logger Flush()?

@yang1070 The proposal is old and at the time it was written the logger worked differently, and now it does`nt match the proposal. So before this implementation SDL flushed all logs always on logger deinitialization, and now configurable timeout shutdown for IGNITION_OFF situation is added.

@VladSemenyuk in that case I'm not sure the proposal is still valid and whether or not we need this change.

the proposal solves the problem that SDL drops off log and give options to continue write the log with maximum allowed time configured by the SDL settings in ini file.

Now (before this PR), SDL get some update, it always flush the log, SDL no long drop off the log, the old problem does not exist any more. The problem may change to SDL takes too much time for flushing the logs? Why we still need to implement the proposal?

The problem may change to SDL takes too much time for flushing the logs? Why we still need to implement the proposal?

@yang1070 Yes. Problem is it takes too much time for flushing the logs in some cases on weak environment.

@VladSemenyuk in that case, I would like to see an update of SDL-117 and get the update proposal approved first

since the default SDL behavior is to flush log,
as in the code change, the setting FlushLogMessagesBeforeShutdown will only impact the normal IGNITION_OFF situation. the name shall be change to something else that is more proper and can describe the situation
or if the name does not change, shall FlushLogMessagesBeforeShutdown=false to force SDL drop off log no matter the what the exit point is?

yang1070 · 2021-07-26T20:55:27Z

src/components/utils/include/utils/logger/logger_impl.h

  LoopThreadPtr<LogMessageLoopThread> loop_thread_;
  bool use_message_loop_thread_;
+  TimePoint flush_logs_time_point_;


what's the default value of flush_logs_time_point_ if it is not initialized?

Default constructor, creates a time_point representing the Clock's epoch (i.e., time_since_epoch() is zero).

@VladSemenyuk Got it. Thank you

yang1070 · 2021-07-26T21:44:23Z

src/appMain/main.cc

@@ -162,6 +162,7 @@ int32_t main(int32_t argc, char** argv) {
 #endif  // LOG4CXX_LOGGER


it is marked "not ready for review" in the PR summary , is that correct?

This means the PR is not ready for the Project Maintainer review.

@KhrystynaDubovyk got it, thank you

yang1070 · 2021-07-26T22:13:58Z

src/appMain/smartDeviceLink.ini

@@ -411,3 +411,9 @@ RpcPassThroughTimeout = 10000
 [RCModuleConsent]
 ; The period (in days) after which consent for module_id should be removed.
 PeriodForConsentExpiration = 30
+
+[Logger]
+; Write all logs in queue to file system before shutdown 


the comment "write all logs in queue" may not be correct, and it is misleading

yang1070 · 2021-07-27T14:49:34Z

src/components/config_profile/include/config_profile/profile.h

+   */
+  bool flush_log_messages_before_shutdown() const OVERRIDE;
+  /**
+   * @brief  Returns waximum time to wait for writing all data before exit SDL


typo waximum

@yang1070 Done

yang1070 · 2021-07-27T14:50:09Z

src/components/config_profile/include/config_profile/profile.h

@@ -589,6 +591,18 @@ class Profile : public protocol_handler::ProtocolHandlerSettings,

  const std::vector<std::string>& embedded_services() const OVERRIDE;
  const std::string hmi_origin_id() const OVERRIDE;
+
+  /**
+   * @brief  Returns true if writing all logs to file system before shutdown is


please remove "all" in comment

@yang1070 Done

src/appMain/smartDeviceLink.ini

src/components/utils/include/utils/logger/logger_settings.h

src/components/config_profile/include/config_profile/profile.h

Add Logger config settings with params for configurable time before s…

1fdc52f

…hutdown. Update SDL config. Extend LoggerImpl class for shutdown configuration. Extend MessageLoopThread, MessageQueque and ConditionlVariable classes with new wait mechanism.

VladSemenyuk mentioned this pull request Jul 21, 2021

Configurable time before shutdown smartdevicelink/sdl_core_guides#130

Open

1 task

dboltovskyi changed the title ~~Configurable time before shutdown~~ [SDL-0117] Configurable time before shutdown Jul 23, 2021

yang1070 reviewed Jul 26, 2021

View reviewed changes

Fix comment for ini option

3c464af

yang1070 reviewed Jul 27, 2021

View reviewed changes

Fix review comments

7bbec3e

ShobhitAd requested changes Jul 29, 2021

View reviewed changes

Fix review comments

573cbe7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDL-0117] Configurable time before shutdown #3740

[SDL-0117] Configurable time before shutdown #3740

VladSemenyuk commented Jul 19, 2021 •

edited

yang1070 Jul 26, 2021

VladSemenyuk Jul 27, 2021

yang1070 Jul 27, 2021

VladSemenyuk Jul 27, 2021

yang1070 Jul 27, 2021 •

edited

VladSemenyuk Jul 27, 2021 •

edited

yang1070 Jul 27, 2021

VladSemenyuk Jul 27, 2021 •

edited

yang1070 Jul 27, 2021 •

edited

yang1070 Jul 27, 2021 •

edited

yang1070 Jul 26, 2021

VladSemenyuk Jul 27, 2021

yang1070 Jul 27, 2021

yang1070 Jul 26, 2021

KhrystynaDubovyk Jul 27, 2021

yang1070 Jul 27, 2021 •

edited

yang1070 Jul 26, 2021

VladSemenyuk Jul 27, 2021

yang1070 Jul 27, 2021

VladSemenyuk Jul 27, 2021

yang1070 Jul 27, 2021

VladSemenyuk Jul 27, 2021

		@@ -162,6 +162,7 @@ int32_t main(int32_t argc, char** argv) {
		#endif // LOG4CXX_LOGGER

[SDL-0117] Configurable time before shutdown #3740

Are you sure you want to change the base?

[SDL-0117] Configurable time before shutdown #3740

Conversation

VladSemenyuk commented Jul 19, 2021 • edited

Risk

Testing Plan

Summary

CLA

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yang1070 Jul 27, 2021 • edited

Choose a reason for hiding this comment

VladSemenyuk Jul 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladSemenyuk Jul 27, 2021 • edited

Choose a reason for hiding this comment

yang1070 Jul 27, 2021 • edited

Choose a reason for hiding this comment

yang1070 Jul 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yang1070 Jul 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladSemenyuk commented Jul 19, 2021 •

edited

yang1070 Jul 27, 2021 •

edited

VladSemenyuk Jul 27, 2021 •

edited

VladSemenyuk Jul 27, 2021 •

edited

yang1070 Jul 27, 2021 •

edited

yang1070 Jul 27, 2021 •

edited

yang1070 Jul 27, 2021 •

edited