You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by "Nathan Marz (JIRA)" <ji...@apache.org> on 2009/10/08 00:01:32 UTC
[jira] Created: (HADOOP-6298) BytesWritable#getBytes is a bad name
that leads to programming mistakes
BytesWritable#getBytes is a bad name that leads to programming mistakes
-----------------------------------------------------------------------
Key: HADOOP-6298
URL: https://issues.apache.org/jira/browse/HADOOP-6298
Project: Hadoop Common
Issue Type: Improvement
Affects Versions: 0.20.1
Reporter: Nathan Marz
Pretty much everyone at Rapleaf who has worked with Hadoop has misused BytesWritable#getBytes at some point, not expecting the byte array to be padded. I think we can completely alleviate these programming mistakes by deprecating and renaming this method (again) to be more descriptive. I propose "getPaddedBytes()" or "getPaddedValue()". It would also be helpful to have a helper method "getNonPaddedValue()" that makes a copy into a non-padded byte array.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-6298) BytesWritable#getBytes is a bad
name that leads to programming mistakes
Posted by "Nathan Marz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-6298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12763841#action_12763841 ]
Nathan Marz commented on HADOOP-6298:
-------------------------------------
Yes - I'm proposing solely a rename, not an API change. The behavior of the method is exactly as it should be for performance reasons. I'm ok with leaving out "getNonPaddedValue" and leaving it up to the user to do the copy if they need it - this idea was just for programmer convenience but I can see how it could be accidentally misused.
I understand that it's clearly documented - the problem is, no one looks at the JavaDoc for these seemingly "obvious" methods. Can we at least agree that the name is confusing? Myself and all my coworkers who have worked with Hadoop have made this mistake.
> BytesWritable#getBytes is a bad name that leads to programming mistakes
> -----------------------------------------------------------------------
>
> Key: HADOOP-6298
> URL: https://issues.apache.org/jira/browse/HADOOP-6298
> Project: Hadoop Common
> Issue Type: Improvement
> Affects Versions: 0.20.1
> Reporter: Nathan Marz
>
> Pretty much everyone at Rapleaf who has worked with Hadoop has misused BytesWritable#getBytes at some point, not expecting the byte array to be padded. I think we can completely alleviate these programming mistakes by deprecating and renaming this method (again) to be more descriptive. I propose "getPaddedBytes()" or "getPaddedValue()". It would also be helpful to have a helper method "getNonPaddedValue()" that makes a copy into a non-padded byte array.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-6298) BytesWritable#getBytes is a bad
name that leads to programming mistakes
Posted by "Chris Douglas (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-6298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12763568#action_12763568 ]
Chris Douglas commented on HADOOP-6298:
---------------------------------------
bq. I don't think this proposal is about changing the API, it's about renaming the method
That's a fine distinction. Forcing all the users of BytesWritable- and they are legion- to change their calls to the most preferred of three identical methods isn't clearing up anything.
Besides, I disagree with the premise. Almost all non-trivial Writables reuse their storage for performance reasons, so querying which of the data are valid for an array is routine. It's consistent with how it's used in map/reduce. It makes sense.
> BytesWritable#getBytes is a bad name that leads to programming mistakes
> -----------------------------------------------------------------------
>
> Key: HADOOP-6298
> URL: https://issues.apache.org/jira/browse/HADOOP-6298
> Project: Hadoop Common
> Issue Type: Improvement
> Affects Versions: 0.20.1
> Reporter: Nathan Marz
>
> Pretty much everyone at Rapleaf who has worked with Hadoop has misused BytesWritable#getBytes at some point, not expecting the byte array to be padded. I think we can completely alleviate these programming mistakes by deprecating and renaming this method (again) to be more descriptive. I propose "getPaddedBytes()" or "getPaddedValue()". It would also be helpful to have a helper method "getNonPaddedValue()" that makes a copy into a non-padded byte array.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-6298) BytesWritable#getBytes is a bad
name that leads to programming mistakes
Posted by "Owen O'Malley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-6298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12784894#action_12784894 ]
Owen O'Malley commented on HADOOP-6298:
---------------------------------------
This *would be* an API change. Renaming methods means that all of the clients need to change their code. I agree the name is unfortunate, precisely because String.toBytes() has different semantics.
Also note that this is both in Text and BytesWritable, so you really would need to do the same change on both of them.
I still don't think the pain is worth the gain. I'd suggest adding a new method that does what you want. Maybe something like:
{code}
/**
* Get a copy of the bytes that is exactly the length of the data.
*/
public byte[] getSlicedBytes() {
byte[] result = new byte[length];
System.arraycopy(bytes, 0, result, 0, length);
return result;
}
{code}
Once you have the method, you can change your examples to use it and your co-workers will follow suit. Do note that it has a cost in terms of performance...
> BytesWritable#getBytes is a bad name that leads to programming mistakes
> -----------------------------------------------------------------------
>
> Key: HADOOP-6298
> URL: https://issues.apache.org/jira/browse/HADOOP-6298
> Project: Hadoop Common
> Issue Type: Improvement
> Affects Versions: 0.20.1
> Reporter: Nathan Marz
>
> Pretty much everyone at Rapleaf who has worked with Hadoop has misused BytesWritable#getBytes at some point, not expecting the byte array to be padded. I think we can completely alleviate these programming mistakes by deprecating and renaming this method (again) to be more descriptive. I propose "getPaddedBytes()" or "getPaddedValue()". It would also be helpful to have a helper method "getNonPaddedValue()" that makes a copy into a non-padded byte array.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-6298) BytesWritable#getBytes is a bad
name that leads to programming mistakes
Posted by "Owen O'Malley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-6298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12763598#action_12763598 ]
Owen O'Malley commented on HADOOP-6298:
---------------------------------------
I think this is a very bad idea. It introduces an incompatible change in a key interface. The method is *clearly* documented. I propose we close this as won't fix.
> BytesWritable#getBytes is a bad name that leads to programming mistakes
> -----------------------------------------------------------------------
>
> Key: HADOOP-6298
> URL: https://issues.apache.org/jira/browse/HADOOP-6298
> Project: Hadoop Common
> Issue Type: Improvement
> Affects Versions: 0.20.1
> Reporter: Nathan Marz
>
> Pretty much everyone at Rapleaf who has worked with Hadoop has misused BytesWritable#getBytes at some point, not expecting the byte array to be padded. I think we can completely alleviate these programming mistakes by deprecating and renaming this method (again) to be more descriptive. I propose "getPaddedBytes()" or "getPaddedValue()". It would also be helpful to have a helper method "getNonPaddedValue()" that makes a copy into a non-padded byte array.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-6298) BytesWritable#getBytes is a bad
name that leads to programming mistakes
Posted by "Chris Douglas (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-6298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12763295#action_12763295 ]
Chris Douglas commented on HADOOP-6298:
---------------------------------------
bq. I propose "getPaddedBytes()" or "getPaddedValue()". It would also be helpful to have a helper method "getNonPaddedValue()" that makes a copy into a non-padded byte array.
Changing BytesWritable's API is a non-starter. Keys and values are reused and allocating new backing for each record is not an endurable expense. Marking valid data is typical for Writables, e.g. BytesWritable and Text.
If this issue modified the javadoc to make this more prominent, that would be reasonable.
> BytesWritable#getBytes is a bad name that leads to programming mistakes
> -----------------------------------------------------------------------
>
> Key: HADOOP-6298
> URL: https://issues.apache.org/jira/browse/HADOOP-6298
> Project: Hadoop Common
> Issue Type: Improvement
> Affects Versions: 0.20.1
> Reporter: Nathan Marz
>
> Pretty much everyone at Rapleaf who has worked with Hadoop has misused BytesWritable#getBytes at some point, not expecting the byte array to be padded. I think we can completely alleviate these programming mistakes by deprecating and renaming this method (again) to be more descriptive. I propose "getPaddedBytes()" or "getPaddedValue()". It would also be helpful to have a helper method "getNonPaddedValue()" that makes a copy into a non-padded byte array.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-6298) BytesWritable#getBytes is a bad
name that leads to programming mistakes
Posted by "Tom White (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-6298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12763520#action_12763520 ]
Tom White commented on HADOOP-6298:
-----------------------------------
I don't think this proposal is about changing the API, it's about renaming the method to more accurately describe its contract. Text.getBytes() behaves differently to String.getBytes(). It is a problem that trips up users; see, for example, http://www.nabble.com/can%27t-read-the-SequenceFile-correctly-td21866960.html.
We could deprecate getBytes() (on BinaryComparable and its subclasses BytesWritable and Text) in 0.22 and create getPaddedBytes() as Nathan suggests, which is identical in functionality. Then in the next release we would remove getBytes(). This change would not have any impact on efficiency, since it is purely a rename.
Nathan, what's the use case for getNonPaddedValue()? It's possible that by exposing it, it becomes easy to write an inefficient program since copying in maps or reduces is normally expensive.
> BytesWritable#getBytes is a bad name that leads to programming mistakes
> -----------------------------------------------------------------------
>
> Key: HADOOP-6298
> URL: https://issues.apache.org/jira/browse/HADOOP-6298
> Project: Hadoop Common
> Issue Type: Improvement
> Affects Versions: 0.20.1
> Reporter: Nathan Marz
>
> Pretty much everyone at Rapleaf who has worked with Hadoop has misused BytesWritable#getBytes at some point, not expecting the byte array to be padded. I think we can completely alleviate these programming mistakes by deprecating and renaming this method (again) to be more descriptive. I propose "getPaddedBytes()" or "getPaddedValue()". It would also be helpful to have a helper method "getNonPaddedValue()" that makes a copy into a non-padded byte array.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-6298) BytesWritable#getBytes is a bad
name that leads to programming mistakes
Posted by "David Rosenstrauch (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-6298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12902051#action_12902051 ]
David Rosenstrauch commented on HADOOP-6298:
--------------------------------------------
We just got hit with this problem as well, and lost several hours on this due to getBytes giving me back an 18-byte array instead of the expected 12.
I realize this method is part of the API out in the field now, and so likely can't be changed. But I think there should be *some* solution implemented for this. i.e., some new method that's a variation on the name "getBytes" that provides the expected behavior.
It's just asking for trouble leaving this little land mine out there for developers to keep stumbling upon.
> BytesWritable#getBytes is a bad name that leads to programming mistakes
> -----------------------------------------------------------------------
>
> Key: HADOOP-6298
> URL: https://issues.apache.org/jira/browse/HADOOP-6298
> Project: Hadoop Common
> Issue Type: Improvement
> Affects Versions: 0.20.1
> Reporter: Nathan Marz
>
> Pretty much everyone at Rapleaf who has worked with Hadoop has misused BytesWritable#getBytes at some point, not expecting the byte array to be padded. I think we can completely alleviate these programming mistakes by deprecating and renaming this method (again) to be more descriptive. I propose "getPaddedBytes()" or "getPaddedValue()". It would also be helpful to have a helper method "getNonPaddedValue()" that makes a copy into a non-padded byte array.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.