Hi all

I have question on some code (taken from HLog) see below
  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
oldestWALseqid,
      final Map<byte [], Long> regionsToSeqids) {
    //  This method is static so it can be unit tested the easier.
    List<byte []> regions = null;
    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
      if (e.getValue().longValue() <= oldestWALseqid) {
        if (regions == null) regions = new ArrayList<byte []>();
        regions.add(e.getKey());
      }
    }
    return regions == null?
      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
  }

Shouldn't be better to remove the if in the loop doing as follow?

  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
oldestWALseqid,
      final Map<byte [], Long> regionsToSeqids) {
    //  This method is static so it can be unit tested the easier.
    List<byte []> regions = new ArrayList<byte []>();
    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
      if (e.getValue().longValue() <= oldestWALseqid) {
        //if (regions == null) regions = new ArrayList<byte []>();
        regions.add(e.getKey());
      }
    }
    return regions.size() == 0?
      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
  }

regards,

Mikael.S

Yeah that'd work too. File a JIRA with the change?

On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:

> Hi all
>
> I have question on some code (taken from HLog) see below
>
>
>  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> oldestWALseqid,
>      final Map<byte [], Long> regionsToSeqids) {
>    //  This method is static so it can be unit tested the easier.
>    List<byte []> regions = null;
>    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>      if (e.getValue().longValue() <= oldestWALseqid) {
>        if (regions == null) regions = new ArrayList<byte []>();
>        regions.add(e.getKey());
>      }
>    }
>    return regions == null?
>      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>  }
>
> Shouldn't be better to remove the if in the loop doing as follow?
>
>  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> oldestWALseqid,
>      final Map<byte [], Long> regionsToSeqids) {
>    //  This method is static so it can be unit tested the easier.
>    List<byte []> regions = new ArrayList<byte []>();
>    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>      if (e.getValue().longValue() <= oldestWALseqid) {
>        //if (regions == null) regions = new ArrayList<byte []>();
>        regions.add(e.getKey());
>      }
>    }
>    return regions.size() == 0?
>      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>  }
>
> regards,
>
> Mikael.S

NP I'll fill it
On Dec 29, 2011 10:52 AM, "Harsh J" <[EMAIL PROTECTED]> wrote:

> Yeah that'd work too. File a JIRA with the change?
>
> On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:
>
> > Hi all
> >
> > I have question on some code (taken from HLog) see below
> >
> >
> >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> > oldestWALseqid,
> >      final Map<byte [], Long> regionsToSeqids) {
> >    //  This method is static so it can be unit tested the easier.
> >    List<byte []> regions = null;
> >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> >      if (e.getValue().longValue() <= oldestWALseqid) {
> >        if (regions == null) regions = new ArrayList<byte []>();
> >        regions.add(e.getKey());
> >      }
> >    }
> >    return regions == null?
> >      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
> >  }
> >
> > Shouldn't be better to remove the if in the loop doing as follow?
> >
> >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> > oldestWALseqid,
> >      final Map<byte [], Long> regionsToSeqids) {
> >    //  This method is static so it can be unit tested the easier.
> >    List<byte []> regions = new ArrayList<byte []>();
> >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> >      if (e.getValue().longValue() <= oldestWALseqid) {
> >        //if (regions == null) regions = new ArrayList<byte []>();
> >        regions.add(e.getKey());
> >      }
> >    }
> >    return regions.size() == 0?
> >      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
> >  }
> >
> > regards,
> >
> > Mikael.S
>
>

The only downside is that the ArrayList now is always created, even if it is not needed, thereby producing unnecessary garbage.
I have not bearing how frequently we'll get here and find no relevant regions, though.
-- Lars
----- Original Message -----
From: Harsh J <[EMAIL PROTECTED]>
To: [EMAIL PROTECTED]
Cc:
Sent: Thursday, December 29, 2011 12:51 AM
Subject: Re: question regarding code

Yeah that'd work too. File a JIRA with the change?

On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:

> Hi all
>
> I have question on some code (taken from HLog) see below
>
>
>  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> oldestWALseqid,
>      final Map<byte [], Long> regionsToSeqids) {
>    //  This method is static so it can be unit tested the easier.
>    List<byte []> regions = null;
>    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>      if (e.getValue().longValue() <= oldestWALseqid) {
>        if (regions == null) regions = new ArrayList<byte []>();
>        regions.add(e.getKey());
>      }
>    }
>    return regions == null?
>      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>  }
>
> Shouldn't be better to remove the if in the loop doing as follow?
>
>  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> oldestWALseqid,
>      final Map<byte [], Long> regionsToSeqids) {
>    //  This method is static so it can be unit tested the easier.
>    List<byte []> regions = new ArrayList<byte []>();
>    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>      if (e.getValue().longValue() <= oldestWALseqid) {
>        //if (regions == null) regions = new ArrayList<byte []>();
>        regions.add(e.getKey());
>      }
>    }
>    return regions.size() == 0?
>      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>  }
>
> regards,
>
> Mikael.S

Lars hi

This method will be called anytime that old log exist and might be cleaned
(HLog.cleanOldsLogs()) and there are too much logs.
It is called from LogRoller thread, Hlog creation, and metaUtils.shutdown()
(code in 0.90.1 - also present at least in those classes in trunk),
The creation of Arraylist is approx 24 bytes (8 for Arraylist object, 12
for the array member and 4 for the int member) if I didn't forgot
something, nevertheless the "if" will not be checked for each region having
an older log file (which will occurs a lot if you have heavy writes
scenarios).
This is true that this will create garbage (in case it will not be used)
but this is a short lived object, it will be collected immediately with a
minor garbage collection. It would have be more problematic if the object
was long lived.

Anyway you are the one to decide, if you accept a jira, i will open one,
just let me know.

Regards,
Mikael.S
On Thu, Dec 29, 2011 at 6:54 PM, lars hofhansl <[EMAIL PROTECTED]> wrote:

> The only downside is that the ArrayList now is always created, even if it
> is not needed, thereby producing unnecessary garbage.
> I have not bearing how frequently we'll get here and find no relevant
> regions, though.
>
>
> -- Lars
>
>
> ----- Original Message -----
> From: Harsh J <[EMAIL PROTECTED]>
> To: [EMAIL PROTECTED]
> Cc:
> Sent: Thursday, December 29, 2011 12:51 AM
> Subject: Re: question regarding code
>
> Yeah that'd work too. File a JIRA with the change?
>
> On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:
>
> > Hi all
> >
> > I have question on some code (taken from HLog) see below
> >
> >
> >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> > oldestWALseqid,
> >      final Map<byte [], Long> regionsToSeqids) {
> >    //  This method is static so it can be unit tested the easier.
> >    List<byte []> regions = null;
> >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> >      if (e.getValue().longValue() <= oldestWALseqid) {
> >        if (regions == null) regions = new ArrayList<byte []>();
> >        regions.add(e.getKey());
> >      }
> >    }
> >    return regions == null?
> >      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
> >  }
> >
> > Shouldn't be better to remove the if in the loop doing as follow?
> >
> >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> > oldestWALseqid,
> >      final Map<byte [], Long> regionsToSeqids) {
> >    //  This method is static so it can be unit tested the easier.
> >    List<byte []> regions = new ArrayList<byte []>();
> >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> >      if (e.getValue().longValue() <= oldestWALseqid) {
> >        //if (regions == null) regions = new ArrayList<byte []>();
> >        regions.add(e.getKey());
> >      }
> >    }
> >    return regions.size() == 0?
> >      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
> >  }
> >
> > regards,
> >
> > Mikael.S
>

--
Mikael.S

Lars,

Yes, but that vs. unnecessary if-checks in every loop, makes it okay?
Unless these are little nitpicky worries and JVMs do a lot better
magic inside :)

On Thu, Dec 29, 2011 at 10:24 PM, lars hofhansl <[EMAIL PROTECTED]> wrote:
> The only downside is that the ArrayList now is always created, even if it is not needed, thereby producing unnecessary garbage.
> I have not bearing how frequently we'll get here and find no relevant regions, though.
>
>
> -- Lars
>
>
> ----- Original Message -----
> From: Harsh J <[EMAIL PROTECTED]>
> To: [EMAIL PROTECTED]
> Cc:
> Sent: Thursday, December 29, 2011 12:51 AM
> Subject: Re: question regarding code
>
> Yeah that'd work too. File a JIRA with the change?
>
> On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:
>
>> Hi all
>>
>> I have question on some code (taken from HLog) see below
>>
>>
>>  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
>> oldestWALseqid,
>>      final Map<byte [], Long> regionsToSeqids) {
>>    //  This method is static so it can be unit tested the easier.
>>    List<byte []> regions = null;
>>    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>>      if (e.getValue().longValue() <= oldestWALseqid) {
>>        if (regions == null) regions = new ArrayList<byte []>();
>>        regions.add(e.getKey());
>>      }
>>    }
>>    return regions == null?
>>      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>>  }
>>
>> Shouldn't be better to remove the if in the loop doing as follow?
>>
>>  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
>> oldestWALseqid,
>>      final Map<byte [], Long> regionsToSeqids) {
>>    //  This method is static so it can be unit tested the easier.
>>    List<byte []> regions = new ArrayList<byte []>();
>>    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>>      if (e.getValue().longValue() <= oldestWALseqid) {
>>        //if (regions == null) regions = new ArrayList<byte []>();
>>        regions.add(e.getKey());
>>      }
>>    }
>>    return regions.size() == 0?
>>      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>>  }
>>
>> regards,
>>
>> Mikael.S

--
Harsh J

Mikael:
I think you can go ahead and open a JIRA.

Happy New Year.

On Thu, Dec 29, 2011 at 4:15 PM, Mikael Sitruk <[EMAIL PROTECTED]>wrote:

> Lars hi
>
> This method will be called anytime that old log exist and might be cleaned
> (HLog.cleanOldsLogs()) and there are too much logs.
> It is called from LogRoller thread, Hlog creation, and metaUtils.shutdown()
> (code in 0.90.1 - also present at least in those classes in trunk),
> The creation of Arraylist is approx 24 bytes (8 for Arraylist object, 12
> for the array member and 4 for the int member) if I didn't forgot
> something, nevertheless the "if" will not be checked for each region having
> an older log file (which will occurs a lot if you have heavy writes
> scenarios).
> This is true that this will create garbage (in case it will not be used)
> but this is a short lived object, it will be collected immediately with a
> minor garbage collection. It would have be more problematic if the object
> was long lived.
>
> Anyway you are the one to decide, if you accept a jira, i will open one,
> just let me know.
>
> Regards,
> Mikael.S
>
>
> On Thu, Dec 29, 2011 at 6:54 PM, lars hofhansl <[EMAIL PROTECTED]>
> wrote:
>
> > The only downside is that the ArrayList now is always created, even if it
> > is not needed, thereby producing unnecessary garbage.
> > I have not bearing how frequently we'll get here and find no relevant
> > regions, though.
> >
> >
> > -- Lars
> >
> >
> > ----- Original Message -----
> > From: Harsh J <[EMAIL PROTECTED]>
> > To: [EMAIL PROTECTED]
> > Cc:
> > Sent: Thursday, December 29, 2011 12:51 AM
> > Subject: Re: question regarding code
> >
> > Yeah that'd work too. File a JIRA with the change?
> >
> > On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:
> >
> > > Hi all
> > >
> > > I have question on some code (taken from HLog) see below
> > >
> > >
> > >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> > > oldestWALseqid,
> > >      final Map<byte [], Long> regionsToSeqids) {
> > >    //  This method is static so it can be unit tested the easier.
> > >    List<byte []> regions = null;
> > >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> > >      if (e.getValue().longValue() <= oldestWALseqid) {
> > >        if (regions == null) regions = new ArrayList<byte []>();
> > >        regions.add(e.getKey());
> > >      }
> > >    }
> > >    return regions == null?
> > >      null: regions.toArray(new byte [][]
> {HConstants.EMPTY_BYTE_ARRAY});
> > >  }
> > >
> > > Shouldn't be better to remove the if in the loop doing as follow?
> > >
> > >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> > > oldestWALseqid,
> > >      final Map<byte [], Long> regionsToSeqids) {
> > >    //  This method is static so it can be unit tested the easier.
> > >    List<byte []> regions = new ArrayList<byte []>();
> > >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> > >      if (e.getValue().longValue() <= oldestWALseqid) {
> > >        //if (regions == null) regions = new ArrayList<byte []>();
> > >        regions.add(e.getKey());
> > >      }
> > >    }
> > >    return regions.size() == 0?
> > >      null: regions.toArray(new byte [][]
> {HConstants.EMPTY_BYTE_ARRAY});
> > >  }
> > >
> > > regards,
> > >
> > > Mikael.S
> >
>
>
>
> --
> Mikael.S
>

Yeah I think we should change it. Does not look like it's going to be called often enough to make a difference. So better readability is good :)

Harsh J <[EMAIL PROTECTED]> schrieb:

>Lars,
>
>Yes, but that vs. unnecessary if-checks in every loop, makes it okay?
>Unless these are little nitpicky worries and JVMs do a lot better
>magic inside :)
>
>On Thu, Dec 29, 2011 at 10:24 PM, lars hofhansl <[EMAIL PROTECTED]> wrote:
>> The only downside is that the ArrayList now is always created, even if it is not needed, thereby producing unnecessary garbage.
>> I have not bearing how frequently we'll get here and find no relevant regions, though.
>>
>>
>> -- Lars
>>
>>
>> ----- Original Message -----
>> From: Harsh J <[EMAIL PROTECTED]>
>> To: [EMAIL PROTECTED]
>> Cc:
>> Sent: Thursday, December 29, 2011 12:51 AM
>> Subject: Re: question regarding code
>>
>> Yeah that'd work too. File a JIRA with the change?
>>
>> On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:
>>
>>> Hi all
>>>
>>> I have question on some code (taken from HLog) see below
>>>
>>>
>>>  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
>>> oldestWALseqid,
>>>      final Map<byte [], Long> regionsToSeqids) {
>>>    //  This method is static so it can be unit tested the easier.
>>>    List<byte []> regions = null;
>>>    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>>>      if (e.getValue().longValue() <= oldestWALseqid) {
>>>        if (regions == null) regions = new ArrayList<byte []>();
>>>        regions.add(e.getKey());
>>>      }
>>>    }
>>>    return regions == null?
>>>      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>>>  }
>>>
>>> Shouldn't be better to remove the if in the loop doing as follow?
>>>
>>>  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
>>> oldestWALseqid,
>>>      final Map<byte [], Long> regionsToSeqids) {
>>>    //  This method is static so it can be unit tested the easier.
>>>    List<byte []> regions = new ArrayList<byte []>();
>>>    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>>>      if (e.getValue().longValue() <= oldestWALseqid) {
>>>        //if (regions == null) regions = new ArrayList<byte []>();
>>>        regions.add(e.getKey());
>>>      }
>>>    }
>>>    return regions.size() == 0?
>>>      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>>>  }
>>>
>>> regards,
>>>
>>> Mikael.S
>
>
>
>--
>Harsh J

oh no no... please file a jira and help us make HBase better.

Mikael Sitruk <[EMAIL PROTECTED]> schrieb:

>Lars hi
>
>This method will be called anytime that old log exist and might be cleaned
>(HLog.cleanOldsLogs()) and there are too much logs.
>It is called from LogRoller thread, Hlog creation, and metaUtils.shutdown()
>(code in 0.90.1 - also present at least in those classes in trunk),
>The creation of Arraylist is approx 24 bytes (8 for Arraylist object, 12
>for the array member and 4 for the int member) if I didn't forgot
>something, nevertheless the "if" will not be checked for each region having
>an older log file (which will occurs a lot if you have heavy writes
>scenarios).
>This is true that this will create garbage (in case it will not be used)
>but this is a short lived object, it will be collected immediately with a
>minor garbage collection. It would have be more problematic if the object
>was long lived.
>
>Anyway you are the one to decide, if you accept a jira, i will open one,
>just let me know.
>
>Regards,
>Mikael.S
>
>
>On Thu, Dec 29, 2011 at 6:54 PM, lars hofhansl <[EMAIL PROTECTED]> wrote:
>
>> The only downside is that the ArrayList now is always created, even if it
>> is not needed, thereby producing unnecessary garbage.
>> I have not bearing how frequently we'll get here and find no relevant
>> regions, though.
>>
>>
>> -- Lars
>>
>>
>> ----- Original Message -----
>> From: Harsh J <[EMAIL PROTECTED]>
>> To: [EMAIL PROTECTED]
>> Cc:
>> Sent: Thursday, December 29, 2011 12:51 AM
>> Subject: Re: question regarding code
>>
>> Yeah that'd work too. File a JIRA with the change?
>>
>> On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:
>>
>> > Hi all
>> >
>> > I have question on some code (taken from HLog) see below
>> >
>> >
>> >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
>> > oldestWALseqid,
>> >      final Map<byte [], Long> regionsToSeqids) {
>> >    //  This method is static so it can be unit tested the easier.
>> >    List<byte []> regions = null;
>> >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>> >      if (e.getValue().longValue() <= oldestWALseqid) {
>> >        if (regions == null) regions = new ArrayList<byte []>();
>> >        regions.add(e.getKey());
>> >      }
>> >    }
>> >    return regions == null?
>> >      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>> >  }
>> >
>> > Shouldn't be better to remove the if in the loop doing as follow?
>> >
>> >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
>> > oldestWALseqid,
>> >      final Map<byte [], Long> regionsToSeqids) {
>> >    //  This method is static so it can be unit tested the easier.
>> >    List<byte []> regions = new ArrayList<byte []>();
>> >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>> >      if (e.getValue().longValue() <= oldestWALseqid) {
>> >        //if (regions == null) regions = new ArrayList<byte []>();
>> >        regions.add(e.getKey());
>> >      }
>> >    }
>> >    return regions.size() == 0?
>> >      null: regions.toArray(new byte [][] {HConstants.EMPTY_BYTE_ARRAY});
>> >  }
>> >
>> > regards,
>> >
>> > Mikael.S
>>
>
>
>
>--
>Mikael.S

Thanks to all of you, jira -
HBASE-5110<https://issues.apache.org/jira/browse/HBASE-5110> (code
enhancement - remove unnecessary if-checks in every loop in HLog class)

Happy new year for all of you.

Mikael.S

On Fri, Dec 30, 2011 at 6:34 AM, Lars H <[EMAIL PROTECTED]> wrote:

> oh no no... please file a jira and help us make HBase better.
>
> Mikael Sitruk <[EMAIL PROTECTED]> schrieb:
>
> >Lars hi
> >
> >This method will be called anytime that old log exist and might be cleaned
> >(HLog.cleanOldsLogs()) and there are too much logs.
> >It is called from LogRoller thread, Hlog creation, and
> metaUtils.shutdown()
> >(code in 0.90.1 - also present at least in those classes in trunk),
> >The creation of Arraylist is approx 24 bytes (8 for Arraylist object, 12
> >for the array member and 4 for the int member) if I didn't forgot
> >something, nevertheless the "if" will not be checked for each region
> having
> >an older log file (which will occurs a lot if you have heavy writes
> >scenarios).
> >This is true that this will create garbage (in case it will not be used)
> >but this is a short lived object, it will be collected immediately with a
> >minor garbage collection. It would have be more problematic if the object
> >was long lived.
> >
> >Anyway you are the one to decide, if you accept a jira, i will open one,
> >just let me know.
> >
> >Regards,
> >Mikael.S
> >
> >
> >On Thu, Dec 29, 2011 at 6:54 PM, lars hofhansl <[EMAIL PROTECTED]>
> wrote:
> >
> >> The only downside is that the ArrayList now is always created, even if
> it
> >> is not needed, thereby producing unnecessary garbage.
> >> I have not bearing how frequently we'll get here and find no relevant
> >> regions, though.
> >>
> >>
> >> -- Lars
> >>
> >>
> >> ----- Original Message -----
> >> From: Harsh J <[EMAIL PROTECTED]>
> >> To: [EMAIL PROTECTED]
> >> Cc:
> >> Sent: Thursday, December 29, 2011 12:51 AM
> >> Subject: Re: question regarding code
> >>
> >> Yeah that'd work too. File a JIRA with the change?
> >>
> >> On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:
> >>
> >> > Hi all
> >> >
> >> > I have question on some code (taken from HLog) see below
> >> >
> >> >
> >> >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> >> > oldestWALseqid,
> >> >      final Map<byte [], Long> regionsToSeqids) {
> >> >    //  This method is static so it can be unit tested the easier.
> >> >    List<byte []> regions = null;
> >> >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> >> >      if (e.getValue().longValue() <= oldestWALseqid) {
> >> >        if (regions == null) regions = new ArrayList<byte []>();
> >> >        regions.add(e.getKey());
> >> >      }
> >> >    }
> >> >    return regions == null?
> >> >      null: regions.toArray(new byte [][]
> {HConstants.EMPTY_BYTE_ARRAY});
> >> >  }
> >> >
> >> > Shouldn't be better to remove the if in the loop doing as follow?
> >> >
> >> >  static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> >> > oldestWALseqid,
> >> >      final Map<byte [], Long> regionsToSeqids) {
> >> >    //  This method is static so it can be unit tested the easier.
> >> >    List<byte []> regions = new ArrayList<byte []>();
> >> >    for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> >> >      if (e.getValue().longValue() <= oldestWALseqid) {
> >> >        //if (regions == null) regions = new ArrayList<byte []>();
> >> >        regions.add(e.getKey());
> >> >      }
> >> >    }
> >> >    return regions.size() == 0?
> >> >      null: regions.toArray(new byte [][]
> {HConstants.EMPTY_BYTE_ARRAY});
> >> >  }
> >> >
> >> > regards,
> >> >
> >> > Mikael.S
> >>
> >
> >
> >
> >--
> >Mikael.S
>

--
Mikael.S

Mikael:
Can you upload a patch ?

Thanks

On Dec 30, 2011, at 11:06 AM, Mikael Sitruk <[EMAIL PROTECTED]> wrote:

> Thanks to all of you, jira -
> HBASE-5110<https://issues.apache.org/jira/browse/HBASE-5110> (code
> enhancement - remove unnecessary if-checks in every loop in HLog class)
>
> Happy new year for all of you.
>
> Mikael.S
>
> On Fri, Dec 30, 2011 at 6:34 AM, Lars H <[EMAIL PROTECTED]> wrote:
>
>> oh no no... please file a jira and help us make HBase better.
>>
>> Mikael Sitruk <[EMAIL PROTECTED]> schrieb:
>>
>>> Lars hi
>>>
>>> This method will be called anytime that old log exist and might be cleaned
>>> (HLog.cleanOldsLogs()) and there are too much logs.
>>> It is called from LogRoller thread, Hlog creation, and
>> metaUtils.shutdown()
>>> (code in 0.90.1 - also present at least in those classes in trunk),
>>> The creation of Arraylist is approx 24 bytes (8 for Arraylist object, 12
>>> for the array member and 4 for the int member) if I didn't forgot
>>> something, nevertheless the "if" will not be checked for each region
>> having
>>> an older log file (which will occurs a lot if you have heavy writes
>>> scenarios).
>>> This is true that this will create garbage (in case it will not be used)
>>> but this is a short lived object, it will be collected immediately with a
>>> minor garbage collection. It would have be more problematic if the object
>>> was long lived.
>>>
>>> Anyway you are the one to decide, if you accept a jira, i will open one,
>>> just let me know.
>>>
>>> Regards,
>>> Mikael.S
>>>
>>>
>>> On Thu, Dec 29, 2011 at 6:54 PM, lars hofhansl <[EMAIL PROTECTED]>
>> wrote:
>>>
>>>> The only downside is that the ArrayList now is always created, even if
>> it
>>>> is not needed, thereby producing unnecessary garbage.
>>>> I have not bearing how frequently we'll get here and find no relevant
>>>> regions, though.
>>>>
>>>>
>>>> -- Lars
>>>>
>>>>
>>>> ----- Original Message -----
>>>> From: Harsh J <[EMAIL PROTECTED]>
>>>> To: [EMAIL PROTECTED]
>>>> Cc:
>>>> Sent: Thursday, December 29, 2011 12:51 AM
>>>> Subject: Re: question regarding code
>>>>
>>>> Yeah that'd work too. File a JIRA with the change?
>>>>
>>>> On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:
>>>>
>>>>> Hi all
>>>>>
>>>>> I have question on some code (taken from HLog) see below
>>>>>
>>>>>
>>>>> static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
>>>>> oldestWALseqid,
>>>>>     final Map<byte [], Long> regionsToSeqids) {
>>>>>   //  This method is static so it can be unit tested the easier.
>>>>>   List<byte []> regions = null;
>>>>>   for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>>>>>     if (e.getValue().longValue() <= oldestWALseqid) {
>>>>>       if (regions == null) regions = new ArrayList<byte []>();
>>>>>       regions.add(e.getKey());
>>>>>     }
>>>>>   }
>>>>>   return regions == null?
>>>>>     null: regions.toArray(new byte [][]
>> {HConstants.EMPTY_BYTE_ARRAY});
>>>>> }
>>>>>
>>>>> Shouldn't be better to remove the if in the loop doing as follow?
>>>>>
>>>>> static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
>>>>> oldestWALseqid,
>>>>>     final Map<byte [], Long> regionsToSeqids) {
>>>>>   //  This method is static so it can be unit tested the easier.
>>>>>   List<byte []> regions = new ArrayList<byte []>();
>>>>>   for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
>>>>>     if (e.getValue().longValue() <= oldestWALseqid) {
>>>>>       //if (regions == null) regions = new ArrayList<byte []>();
>>>>>       regions.add(e.getKey());
>>>>>     }
>>>>>   }
>>>>>   return regions.size() == 0?
>>>>>     null: regions.toArray(new byte [][]
>> {HConstants.EMPTY_BYTE_ARRAY});
>>>>> }
>>>>>
>>>>> regards,
>>>>>
>>>>> Mikael.S
>>>>
>>>
>>>
>>>
>>> --
>>> Mikael.S
>>
>
>
>
> --
> Mikael.S

it should not be a problem
i'll try
Mikael.S

On Fri, Dec 30, 2011 at 9:10 PM, <[EMAIL PROTECTED]> wrote:

> Mikael:
> Can you upload a patch ?
>
> Thanks
>
>
>
> On Dec 30, 2011, at 11:06 AM, Mikael Sitruk <[EMAIL PROTECTED]>
> wrote:
>
> > Thanks to all of you, jira -
> > HBASE-5110<https://issues.apache.org/jira/browse/HBASE-5110> (code
> > enhancement - remove unnecessary if-checks in every loop in HLog class)
> >
> > Happy new year for all of you.
> >
> > Mikael.S
> >
> > On Fri, Dec 30, 2011 at 6:34 AM, Lars H <[EMAIL PROTECTED]> wrote:
> >
> >> oh no no... please file a jira and help us make HBase better.
> >>
> >> Mikael Sitruk <[EMAIL PROTECTED]> schrieb:
> >>
> >>> Lars hi
> >>>
> >>> This method will be called anytime that old log exist and might be
> cleaned
> >>> (HLog.cleanOldsLogs()) and there are too much logs.
> >>> It is called from LogRoller thread, Hlog creation, and
> >> metaUtils.shutdown()
> >>> (code in 0.90.1 - also present at least in those classes in trunk),
> >>> The creation of Arraylist is approx 24 bytes (8 for Arraylist object,
> 12
> >>> for the array member and 4 for the int member) if I didn't forgot
> >>> something, nevertheless the "if" will not be checked for each region
> >> having
> >>> an older log file (which will occurs a lot if you have heavy writes
> >>> scenarios).
> >>> This is true that this will create garbage (in case it will not be
> used)
> >>> but this is a short lived object, it will be collected immediately
> with a
> >>> minor garbage collection. It would have be more problematic if the
> object
> >>> was long lived.
> >>>
> >>> Anyway you are the one to decide, if you accept a jira, i will open
> one,
> >>> just let me know.
> >>>
> >>> Regards,
> >>> Mikael.S
> >>>
> >>>
> >>> On Thu, Dec 29, 2011 at 6:54 PM, lars hofhansl <[EMAIL PROTECTED]>
> >> wrote:
> >>>
> >>>> The only downside is that the ArrayList now is always created, even if
> >> it
> >>>> is not needed, thereby producing unnecessary garbage.
> >>>> I have not bearing how frequently we'll get here and find no relevant
> >>>> regions, though.
> >>>>
> >>>>
> >>>> -- Lars
> >>>>
> >>>>
> >>>> ----- Original Message -----
> >>>> From: Harsh J <[EMAIL PROTECTED]>
> >>>> To: [EMAIL PROTECTED]
> >>>> Cc:
> >>>> Sent: Thursday, December 29, 2011 12:51 AM
> >>>> Subject: Re: question regarding code
> >>>>
> >>>> Yeah that'd work too. File a JIRA with the change?
> >>>>
> >>>> On 29-Dec-2011, at 2:12 PM, Mikael Sitruk wrote:
> >>>>
> >>>>> Hi all
> >>>>>
> >>>>> I have question on some code (taken from HLog) see below
> >>>>>
> >>>>>
> >>>>> static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> >>>>> oldestWALseqid,
> >>>>>     final Map<byte [], Long> regionsToSeqids) {
> >>>>>   //  This method is static so it can be unit tested the easier.
> >>>>>   List<byte []> regions = null;
> >>>>>   for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> >>>>>     if (e.getValue().longValue() <= oldestWALseqid) {
> >>>>>       if (regions == null) regions = new ArrayList<byte []>();
> >>>>>       regions.add(e.getKey());
> >>>>>     }
> >>>>>   }
> >>>>>   return regions == null?
> >>>>>     null: regions.toArray(new byte [][]
> >> {HConstants.EMPTY_BYTE_ARRAY});
> >>>>> }
> >>>>>
> >>>>> Shouldn't be better to remove the if in the loop doing as follow?
> >>>>>
> >>>>> static byte [][] findMemstoresWithEditsEqualOrOlderThan(final long
> >>>>> oldestWALseqid,
> >>>>>     final Map<byte [], Long> regionsToSeqids) {
> >>>>>   //  This method is static so it can be unit tested the easier.
> >>>>>   List<byte []> regions = new ArrayList<byte []>();
> >>>>>   for (Map.Entry<byte [], Long> e: regionsToSeqids.entrySet()) {
> >>>>>     if (e.getValue().longValue() <= oldestWALseqid) {
> >>>>>       //if (regions == null) regions = new ArrayList<byte []>();
> >>>>>       regions.add(e.getKey());
> >>>>>     }
> >>>>>   }
> >>>>>   return regions.size() == 0?
> >>>>>     null: regions.toArray(new byte [][]

Mikael.S