Skip to content

MR: Fix NPE when InputSplit.getLocations is called on mappers - #1582

Merged
rdblue merged 2 commits into
apache:masterfrom
shardulm94:fix-getlocations-npe
Oct 13, 2020
Merged

MR: Fix NPE when InputSplit.getLocations is called on mappers#1582
rdblue merged 2 commits into
apache:masterfrom
shardulm94:fix-getlocations-npe

Conversation

@shardulm94

@shardulm94 shardulm94 commented Oct 11, 2020

Copy link
Copy Markdown
Contributor

InputSplit.getLocations() can be called on mappers in some cases (Example). Since both locations and conf are transient, the current code produces an NPE as conf is null on the mappers.

The value of locations is not really relevant on the mappers since the tasks have already been distributed. So here we just return ANYWHERE when the conf is null. We can probably be more accurate by serializing the locations values if set, but I don't think its worth it.

java.lang.Exception: java.lang.NullPointerException
    at org.apache.hadoop.mapred.LocalJobRunner$Job.runTasks(LocalJobRunner.java:462)
    at org.apache.hadoop.mapred.LocalJobRunner$Job.run(LocalJobRunner.java:522)
Caused by: java.lang.NullPointerException
    at org.apache.iceberg.mr.mapreduce.IcebergSplit.getLocations(IcebergSplit.java:69)
    .
    .
    .
    at org.apache.pig.backend.hadoop.executionengine.mapReduceLayer.PigSplit.toString(PigSplit.java:465)
    at java.lang.String.valueOf(String.java:2994)
    at java.lang.StringBuilder.append(StringBuilder.java:131)
    at org.apache.hadoop.mapred.MapTask.runNewMapper(MapTask.java:756)
    at org.apache.hadoop.mapred.MapTask.run(MapTask.java:341)
    at org.apache.hadoop.mapred.LocalJobRunner$Job$MapTaskRunnable.run(LocalJobRunner.java:243)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)

I discovered this issue while testing our internal Pig LoadFunc implementation which works with multiple InputFormats. Iceberg's LoadFunc implementation in iceberg-pig does not have this issue as it contains its own InputSplit implementation which does not provide location information (as opposed to the InputSplit implementation in iceberg-mr).

@shardulm94
shardulm94 requested a review from rdsr October 11, 2020 06:08
@shardulm94

Copy link
Copy Markdown
Contributor Author

cc: @guilload

@kbendick kbendick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to me.

And for the example you linked, I agree it seems not necessary to serialize the location data and then have to have special handling of that just for the toString method to have exact locality scheduling information, especially on the mappers once that information should not really be that relevant.

@Override
public String[] getLocations() {
if (locations == null) {
if (locations == null && conf != null) {

@rdsr rdsr Oct 11, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a comment can help here saying implementation of getLocations is only meant to be used during splits computation. getLocations won't be accurate when called on worker nodes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rdblue
rdblue merged commit 1d5f52b into apache:master Oct 13, 2020
@rdblue

rdblue commented Oct 13, 2020

Copy link
Copy Markdown
Contributor

Merging since @rdsr's comment was addressed. Thanks, @shardulm94!

@rdblue rdblue added this to the Java 0.10.0 Release milestone Nov 16, 2020
anuragmantri added a commit to anuragmantri/iceberg that referenced this pull request Jul 25, 2025
…3.5 tests (apache#13040) (apache#1582)

Co-authored-by: Tom Tanaka <43331405+tomtongue@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants