public inbox for [email protected]  

help / color / mirror / Atom feed
From: [email protected] <[email protected]>
To: 'Zhihong Yu' <[email protected]>
Cc: Shinya Kato <[email protected]>
Cc: [email protected] <[email protected]>
Subject: RE: [Proposal] Add foreign-server health checks infrastructure
Date: Mon, 29 Nov 2021 08:51:40 +0000
Message-ID: <TYAPR01MB5866D73C56A4363039175E56F5669@TYAPR01MB5866.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CALNJ-vQWNK6AzpL=5iXLfpr4oRMYdJ72tDYhxLwd=iX5H5XUdQ@mail.gmail.com>
References: <TYAPR01MB58662809E678253B90E82CE5F5889@TYAPR01MB5866.jpnprd01.prod.outlook.com>
	<[email protected]>
	<TYAPR01MB58662CD4FD98AA475B3D10F9F59B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
	<[email protected]>
	<TYAPR01MB58661992162BA7708191EC79F5619@TYAPR01MB5866.jpnprd01.prod.outlook.com>
	<CALNJ-vQWNK6AzpL=5iXLfpr4oRMYdJ72tDYhxLwd=iX5H5XUdQ@mail.gmail.com>

Dear Zhihong,

Thank you for giving comments! I'll post new patches later.

> +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS()  (CheckingRemoteServersHoldoffCount++)
> 
> The macro contains only one operation. Can the macro be removed (with `CheckingRemoteServersHoldoffCount++` inlined) ?

Hmm, these HOLD/RESUME macros are followed HOLD_INTERRUPUST() and HOLD_CANCEL_INTERRUPTS():

```
#define HOLD_INTERRUPTS()  (InterruptHoldoffCount++)

#define RESUME_INTERRUPTS() \
do { \
        Assert(InterruptHoldoffCount > 0); \
        InterruptHoldoffCount--; \
} while(0)

#define HOLD_CANCEL_INTERRUPTS()  (QueryCancelHoldoffCount++)

#define RESUME_CANCEL_INTERRUPTS() \
do { \
        Assert(QueryCancelHoldoffCount > 0); \
        QueryCancelHoldoffCount--; \
} while(0)

#define START_CRIT_SECTION()  (CritSectionCount++)

#define END_CRIT_SECTION() \
do { \
        Assert(CritSectionCount > 0); \
        CritSectionCount--; \
} while(0)
```

So I want to keep the current style. Could you tell me if you have any other reasons?

> +   if (CheckingRemoteServersTimeoutPending && CheckingRemoteServersHoldoffCount != 0)
> +   {
> +       /*
> +        * Skip checking foreign servers while reading messages.
> +        */
> +       InterruptPending = true;
> +   }
> +   else if (CheckingRemoteServersTimeoutPending)
> 
> Would the code be more readable if the above two if blocks be moved inside one enclosing if block (factoring the common condition)?
> 
> +   if (CheckingRemoteServersTimeoutPending)

+1. Will fix.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED





view thread (39+ messages)  latest in thread

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: RE: [Proposal] Add foreign-server health checks infrastructure
  In-Reply-To: <TYAPR01MB5866D73C56A4363039175E56F5669@TYAPR01MB5866.jpnprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox