[DISCUSS] and the NEW Apache Geode 1.7.0 release branch has been created

[Nabarun Nag]

Hello Geode Dev Community,

We have created a new release branch for Apache Geode 1.7.0 -
"release/1.7.0"

Previous branch was deleted and has been replaced with a fresh one.

Please do review and raise any concern with the release branch.

If concerns are raised, we will start with the voting for the release
candidate soon.

Regards
Nabarun Nag

Re: [DISCUSS] and the NEW Apache Geode 1.7.0 release branch has been created

[Nabarun Nag]

CORRECTION: if '*no*' concerns are raised, we will start with the voting
for the release candidate soon.

Regrads
Nabarun Nag

On Fri, Sep 7, 2018 at 9:08 AM Nabarun Nag  wrote:

> Hello Geode Dev Community,
>
> We have created a new release branch for Apache Geode 1.7.0 -
> "release/1.7.0"
>
> Previous branch was deleted and has been replaced with a fresh one.
>
> Please do review and raise any concern with the release branch.
>
> If concerns are raised, we will start with the voting for the release
> candidate soon.
>
> Regards
> Nabarun Nag
>

[DISCUSS] Wrapping log calls in Conditionals like isDebugEnabled, isTraceEnabled, etc.

[Galen O'Sullivan]

Hi all,

I've noticed a pattern in Geode where we wrap a log call in a check at the
same level, such as:

if (logger.isDebugEnabled()) {
  logger.debug("cleaning up incompletely started
DistributionManager due to exception", r);
}

Is there any reason to do this? To my mind, it's an extra conditional that
should essentially be the first check inside `logger.debug(...)` anyways,
and it complicates the code and makes it less readable. I've even seen
places in the code which have `if (logger.isDebugEnabled())
logger.trace(...))` and such.

I would like to propose that unless there is a compelling reason to use
this pattern, we remove all extra checks entirely.

Galen

Re: [DISCUSS] Wrapping log calls in Conditionals like isDebugEnabled, isTraceEnabled, etc.

[Dan Smith]

I think this pattern is a holdover from using log4j 1. In that version, you
added an if check to avoid unnecessary string concatenation if debug was
enabled.


   1. if (logger.isDebugEnabled()) {
   2. logger.debug("Logging in user " + user.getName() + " with
birthday " + user.getBirthdayCalendar());
   3. }


Log4j2 lets you pass a format string, so you can just do this:

logger.debug("Logging in user {} with birthday {}", user.getName(),
user.getBirthdayCalendar());


These examples come from the log4j2 docs:

https://logging.apache.org/log4j/2.0/manual/api.html

-Dan

On Fri, Sep 7, 2018 at 10:50 AM, Galen O'Sullivan 
wrote:

> Hi all,
>
> I've noticed a pattern in Geode where we wrap a log call in a check at the
> same level, such as:
>
> if (logger.isDebugEnabled()) {
>   logger.debug("cleaning up incompletely started
> DistributionManager due to exception", r);
> }
>
> Is there any reason to do this? To my mind, it's an extra conditional that
> should essentially be the first check inside `logger.debug(...)` anyways,
> and it complicates the code and makes it less readable. I've even seen
> places in the code which have `if (logger.isDebugEnabled())
> logger.trace(...))` and such.
>
> I would like to propose that unless there is a compelling reason to use
> this pattern, we remove all extra checks entirely.
>
> Galen
>

Re: [DISCUSS] Wrapping log calls in Conditionals like isDebugEnabled, isTraceEnabled, etc.

[Jacob Barrett]

Checking with logger.isDebugEnabled() it still a good practice for hot
paths to avoid the construction of intermediate object arrays to hold the
var-args. Some logging libraries have fixed argument optimizations for
var-args up to a specific limit. In very hot paths it is nice to
check logger.isDebugEnabled() once into a temp boolean value and then check
that in all the subsequent debug logging statements in the hot path.

-Jake


On Fri, Sep 7, 2018 at 11:18 AM Dan Smith  wrote:

> I think this pattern is a holdover from using log4j 1. In that version, you
> added an if check to avoid unnecessary string concatenation if debug was
> enabled.
>
>
>1. if (logger.isDebugEnabled()) {
>2. logger.debug("Logging in user " + user.getName() + " with
> birthday " + user.getBirthdayCalendar());
>3. }
>
>
> Log4j2 lets you pass a format string, so you can just do this:
>
> logger.debug("Logging in user {} with birthday {}", user.getName(),
> user.getBirthdayCalendar());
>
>
> These examples come from the log4j2 docs:
>
> https://logging.apache.org/log4j/2.0/manual/api.html
>
> -Dan
>
> On Fri, Sep 7, 2018 at 10:50 AM, Galen O'Sullivan 
> wrote:
>
> > Hi all,
> >
> > I've noticed a pattern in Geode where we wrap a log call in a check at
> the
> > same level, such as:
> >
> > if (logger.isDebugEnabled()) {
> >   logger.debug("cleaning up incompletely started
> > DistributionManager due to exception", r);
> > }
> >
> > Is there any reason to do this? To my mind, it's an extra conditional
> that
> > should essentially be the first check inside `logger.debug(...)` anyways,
> > and it complicates the code and makes it less readable. I've even seen
> > places in the code which have `if (logger.isDebugEnabled())
> > logger.trace(...))` and such.
> >
> > I would like to propose that unless there is a compelling reason to use
> > this pattern, we remove all extra checks entirely.
> >
> > Galen
> >
>

Re: [DISCUSS] Wrapping log calls in Conditionals like isDebugEnabled, isTraceEnabled, etc.

[John Blum]

Technically, I think that is SLF4J syntax (but maybe Log4J2 supports this
format now as well; anyway).

Still, you should be careful with log statements like...

 logger.debug("Logging in user {}" + user);

Assuming the User class itself and an "informative" and properly
constructed toString() method.  So use it judiciously and wisely.


On Fri, Sep 7, 2018 at 11:18 AM, Dan Smith  wrote:

> I think this pattern is a holdover from using log4j 1. In that version, you
> added an if check to avoid unnecessary string concatenation if debug was
> enabled.
>
>
>1. if (logger.isDebugEnabled()) {
>2. logger.debug("Logging in user " + user.getName() + " with
> birthday " + user.getBirthdayCalendar());
>3. }
>
>
> Log4j2 lets you pass a format string, so you can just do this:
>
> logger.debug("Logging in user {} with birthday {}", user.getName(),
> user.getBirthdayCalendar());
>
>
> These examples come from the log4j2 docs:
>
> https://logging.apache.org/log4j/2.0/manual/api.html
>
> -Dan
>
> On Fri, Sep 7, 2018 at 10:50 AM, Galen O'Sullivan 
> wrote:
>
> > Hi all,
> >
> > I've noticed a pattern in Geode where we wrap a log call in a check at
> the
> > same level, such as:
> >
> > if (logger.isDebugEnabled()) {
> >   logger.debug("cleaning up incompletely started
> > DistributionManager due to exception", r);
> > }
> >
> > Is there any reason to do this? To my mind, it's an extra conditional
> that
> > should essentially be the first check inside `logger.debug(...)` anyways,
> > and it complicates the code and makes it less readable. I've even seen
> > places in the code which have `if (logger.isDebugEnabled())
> > logger.trace(...))` and such.
> >
> > I would like to propose that unless there is a compelling reason to use
> > this pattern, we remove all extra checks entirely.
> >
> > Galen
> >
>



-- 
-John
john.blum10101 (skype)

Re: [DISCUSS] Wrapping log calls in Conditionals like isDebugEnabled, isTraceEnabled, etc.

[John Blum]

Grrr, meant...

logger.debug("Logging in user {}", user);



On Fri, Sep 7, 2018 at 11:37 AM, John Blum  wrote:

> Technically, I think that is SLF4J syntax (but maybe Log4J2 supports this
> format now as well; anyway).
>
> Still, you should be careful with log statements like...
>
>  logger.debug("Logging in user {}" + user);
>
> Assuming the User class itself and an "informative" and properly
> constructed toString() method.  So use it judiciously and wisely.
>
>
> On Fri, Sep 7, 2018 at 11:18 AM, Dan Smith  wrote:
>
>> I think this pattern is a holdover from using log4j 1. In that version,
>> you
>> added an if check to avoid unnecessary string concatenation if debug was
>> enabled.
>>
>>
>>1. if (logger.isDebugEnabled()) {
>>2. logger.debug("Logging in user " + user.getName() + " with
>> birthday " + user.getBirthdayCalendar());
>>3. }
>>
>>
>> Log4j2 lets you pass a format string, so you can just do this:
>>
>> logger.debug("Logging in user {} with birthday {}", user.getName(),
>> user.getBirthdayCalendar());
>>
>>
>> These examples come from the log4j2 docs:
>>
>> https://logging.apache.org/log4j/2.0/manual/api.html
>>
>> -Dan
>>
>> On Fri, Sep 7, 2018 at 10:50 AM, Galen O'Sullivan 
>> wrote:
>>
>> > Hi all,
>> >
>> > I've noticed a pattern in Geode where we wrap a log call in a check at
>> the
>> > same level, such as:
>> >
>> > if (logger.isDebugEnabled()) {
>> >   logger.debug("cleaning up incompletely started
>> > DistributionManager due to exception", r);
>> > }
>> >
>> > Is there any reason to do this? To my mind, it's an extra conditional
>> that
>> > should essentially be the first check inside `logger.debug(...)`
>> anyways,
>> > and it complicates the code and makes it less readable. I've even seen
>> > places in the code which have `if (logger.isDebugEnabled())
>> > logger.trace(...))` and such.
>> >
>> > I would like to propose that unless there is a compelling reason to use
>> > this pattern, we remove all extra checks entirely.
>> >
>> > Galen
>> >
>>
>
>
>
> --
> -John
> john.blum10101 (skype)
>



-- 
-John
john.blum10101 (skype)

Re: [DISCUSS] Wrapping log calls in Conditionals like isDebugEnabled, isTraceEnabled, etc.

[Anthony Baker]

Check the recommended patterns for java8 [1], I think the lambda syntax makes 
the conditional less needed.

Anthony

[1] https://logging.apache.org/log4j/2.0/manual/api.html 



> On Sep 7, 2018, at 11:38 AM, John Blum  wrote:
> 
> Grrr, meant...
> 
> logger.debug("Logging in user {}", user);
> 
> 
> 
> On Fri, Sep 7, 2018 at 11:37 AM, John Blum  wrote:
> 
>> Technically, I think that is SLF4J syntax (but maybe Log4J2 supports this
>> format now as well; anyway).
>> 
>> Still, you should be careful with log statements like...
>> 
>> logger.debug("Logging in user {}" + user);
>> 
>> Assuming the User class itself and an "informative" and properly
>> constructed toString() method.  So use it judiciously and wisely.
>> 
>> 
>> On Fri, Sep 7, 2018 at 11:18 AM, Dan Smith  wrote:
>> 
>>> I think this pattern is a holdover from using log4j 1. In that version,
>>> you
>>> added an if check to avoid unnecessary string concatenation if debug was
>>> enabled.
>>> 
>>> 
>>>   1. if (logger.isDebugEnabled()) {
>>>   2. logger.debug("Logging in user " + user.getName() + " with
>>> birthday " + user.getBirthdayCalendar());
>>>   3. }
>>> 
>>> 
>>> Log4j2 lets you pass a format string, so you can just do this:
>>> 
>>> logger.debug("Logging in user {} with birthday {}", user.getName(),
>>> user.getBirthdayCalendar());
>>> 
>>> 
>>> These examples come from the log4j2 docs:
>>> 
>>> https://logging.apache.org/log4j/2.0/manual/api.html
>>> 
>>> -Dan
>>> 
>>> On Fri, Sep 7, 2018 at 10:50 AM, Galen O'Sullivan 
>>> wrote:
>>> 
 Hi all,
 
 I've noticed a pattern in Geode where we wrap a log call in a check at
>>> the
 same level, such as:
 
if (logger.isDebugEnabled()) {
  logger.debug("cleaning up incompletely started
 DistributionManager due to exception", r);
}
 
 Is there any reason to do this? To my mind, it's an extra conditional
>>> that
 should essentially be the first check inside `logger.debug(...)`
>>> anyways,
 and it complicates the code and makes it less readable. I've even seen
 places in the code which have `if (logger.isDebugEnabled())
 logger.trace(...))` and such.
 
 I would like to propose that unless there is a compelling reason to use
 this pattern, we remove all extra checks entirely.
 
 Galen
 
>>> 
>> 
>> 
>> 
>> --
>> -John
>> john.blum10101 (skype)
>> 
> 
> 
> 
> -- 
> -John
> john.blum10101 (skype)

Code Reviews

[Jens Deppe]

I came across this article a while back but just re-read it recently and
thought it would be worth sharing.

https://medium.freecodecamp.org/unlearning-toxic-behaviors-in-a-code-review-culture-b7c295452a3c

I don't think we're doing much, if any, of the 'toxic' behavior, but it's
still good to be reminded and also to remember what a 'good' review
consists of.

--Jens

Re: [DISCUSS] Wrapping log calls in Conditionals like isDebugEnabled, isTraceEnabled, etc.

[Nicholas Vallely]

I was just randomly looking into this a couple of days ago as a tangent to
'observability' and came across this Stack Overflow:
https://stackoverflow.com/questions/6504407/is-there-a-need-to-do-a-iflog-isdebugenabled-check
where the first answer is the most succinct in my opinion.

In the geode code that I searched, we are not consistent with our use of
the if(statement) wrapper for debug, though most do have the wrapper.

Nick

On Fri, Sep 7, 2018 at 11:35 AM Jacob Barrett  wrote:

> Checking with logger.isDebugEnabled() it still a good practice for hot
> paths to avoid the construction of intermediate object arrays to hold the
> var-args. Some logging libraries have fixed argument optimizations for
> var-args up to a specific limit. In very hot paths it is nice to
> check logger.isDebugEnabled() once into a temp boolean value and then check
> that in all the subsequent debug logging statements in the hot path.
>
> -Jake
>
>
> On Fri, Sep 7, 2018 at 11:18 AM Dan Smith  wrote:
>
> > I think this pattern is a holdover from using log4j 1. In that version,
> you
> > added an if check to avoid unnecessary string concatenation if debug was
> > enabled.
> >
> >
> >1. if (logger.isDebugEnabled()) {
> >2. logger.debug("Logging in user " + user.getName() + " with
> > birthday " + user.getBirthdayCalendar());
> >3. }
> >
> >
> > Log4j2 lets you pass a format string, so you can just do this:
> >
> > logger.debug("Logging in user {} with birthday {}", user.getName(),
> > user.getBirthdayCalendar());
> >
> >
> > These examples come from the log4j2 docs:
> >
> > https://logging.apache.org/log4j/2.0/manual/api.html
> >
> > -Dan
> >
> > On Fri, Sep 7, 2018 at 10:50 AM, Galen O'Sullivan  >
> > wrote:
> >
> > > Hi all,
> > >
> > > I've noticed a pattern in Geode where we wrap a log call in a check at
> > the
> > > same level, such as:
> > >
> > > if (logger.isDebugEnabled()) {
> > >   logger.debug("cleaning up incompletely started
> > > DistributionManager due to exception", r);
> > > }
> > >
> > > Is there any reason to do this? To my mind, it's an extra conditional
> > that
> > > should essentially be the first check inside `logger.debug(...)`
> anyways,
> > > and it complicates the code and makes it less readable. I've even seen
> > > places in the code which have `if (logger.isDebugEnabled())
> > > logger.trace(...))` and such.
> > >
> > > I would like to propose that unless there is a compelling reason to use
> > > this pattern, we remove all extra checks entirely.
> > >
> > > Galen
> > >
> >
>

New Committers: Use GitBox to Link Apache Account to GitHub Account

[Ryan McMahon]

Hi Everyone,

I just wanted to relay some information to new committers.  We determined
that in order to get write access to the Geode repository, you need to go
to this site and go through the linking process.

https://gitbox.apache.org/setup/

If you don't to do this, you will be added to the "Apache Committers" team
but not the "geode committers" team.

Ryan
mcmellaw...@apache.org

[Spring CI] Spring Data GemFire > Nightly-ApacheGeode > #1033 was SUCCESSFUL (with 2432 tests)

[Spring CI]

---
Spring Data GemFire > Nightly-ApacheGeode > #1033 was successful.
---
Scheduled
2434 tests in total.

https://build.spring.io/browse/SGF-NAG-1033/





--
This message is automatically generated by Atlassian Bamboo