[jira] [Commented] (GOBBLIN-120) Forked converters doesn't know their branch id

["Birger Kamp (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GOBBLIN-120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16294277#comment-16294277 ] 

Birger Kamp commented on GOBBLIN-120:
-------------------------------------

I created a PR for it: https://github.com/apache/incubator-gobblin/pull/2215

> Forked converters doesn't know their branch id
> ----------------------------------------------
>
>                 Key: GOBBLIN-120
>                 URL: https://issues.apache.org/jira/browse/GOBBLIN-120
>             Project: Apache Gobblin
>          Issue Type: Bug
>            Reporter: Abhishek Tiwari
>              Labels: Bug:Generic, Core:Fork
>
> Been having issues with supplying my forked converters with individual settings and finally found the error. The property for the forked branch index wasn't set in the TaskState that was supplied to the forked converter.
> This is from the constructor in gobblin.runtime.Fork
> ```
> this.forkTaskState = branches > 1 ? new TaskState(this.taskState) : this.taskState;
> this.taskId = this.taskState.getTaskId();
> this.branches = branches;
> this.index = index;
> this.converter = this.closer.register(new MultiConverter(this.taskContext.getConverters(this.index, this.forkTaskState)));
> this.convertedSchema = Optional.fromNullable(this.converter.convertSchema(schema, this.taskState));
> ```
> The first row is relevant as here we create a copy of the TaskState for this specific fork. The last rows are the root of the issue. The branch index property is set through the getConverters method in this following row on the _forkTaskState_ object:
> `this.converter = this.closer.register(new MultiConverter(this.taskContext.getConverters(this.index, this.forkTaskState)));`
> But the forked schema conversion is supplied with the original _taskState_, and not the _forkTaskState_ which had the branch id set:
> `this.convertedSchema = Optional.fromNullable(this.converter.convertSchema(schema, this.taskState));`
> I don't know if this is intentional. To me it seems like a simple mistake from the coders, and that the forked convertSchema method should in fact be supplied with forkTaskState instead of the original taskState.
> Edit:
> The same goes for when converter.convertRecord is called in processRecords() further down in Fork.java. It is also supplied with the non-forked version of the TaskState.
>  
> *Github Url* : https://github.com/linkedin/gobblin/issues/848 
> *Github Reporter* : *Zogtark* 
> *Github Created At* : 2016-03-15T13:42:16Z 
> *Github Updated At* : 2017-01-12T04:50:10Z 
> h3. Comments 
> ----
> [~stakiar] wrote on 2016-03-17T02:16:41Z : @Zogtark yes that seems like a bug, `this.forkTaskState` should be used instead of `this.taskState` in both locations you mentioned. The fix should be simple.
>  
>  
> *Github Url* : https://github.com/linkedin/gobblin/issues/848#issuecomment-197656243



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)