You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Mikael Sitruk <mi...@gmail.com> on 2011/12/29 09:42:48 UTC
question regarding code
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
Re: question regarding code
Posted by Ted Yu <yu...@gmail.com>.
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 <mi...@gmail.com>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 <lh...@yahoo.com>
> 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 <ha...@cloudera.com>
> > To: dev@hbase.apache.org
> > 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
>
Re: question regarding code
Posted by Mikael Sitruk <mi...@gmail.com>.
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 <lh...@yahoo.com> 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 <ha...@cloudera.com>
> To: dev@hbase.apache.org
> 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
Re: question regarding code
Posted by Harsh J <ha...@cloudera.com>.
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 <lh...@yahoo.com> 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 <ha...@cloudera.com>
> To: dev@hbase.apache.org
> 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
Re: question regarding code
Posted by lars hofhansl <lh...@yahoo.com>.
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 <ha...@cloudera.com>
To: dev@hbase.apache.org
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
Re: question regarding code
Posted by Mikael Sitruk <mi...@gmail.com>.
NP I'll fill it
On Dec 29, 2011 10:52 AM, "Harsh J" <ha...@cloudera.com> 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
>
>
Re: question regarding code
Posted by Harsh J <ha...@cloudera.com>.
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