APEXMALHAR-2195 - Fix LineReaderContext last record #372
Conversation
yogidevendra
commented
Aug 16, 2016
yogidevendra
commented
- Changes in the test app
|
@amberarrow Could you please review this? |
|
Ok, over the weekend. |
| @@ -89,15 +89,20 @@ public void testDelimitedRecords() throws Exception | |||
| lma.prepareDAG(app, conf); | |||
| LocalMode.Controller lc = lma.getController(); | |||
| lc.setHeartbeatMonitoringEnabled(true); | |||
| lc.runAsync(); | |||
| lc.run(10 * 1000); | |||
| LOG.debug("Waiting for app to finish"); | |||
| Thread.sleep(1000 * 1); | |||
| lc.shutdown(); | |||
There was a problem hiding this comment.
lc.shutdown is not required if you are using lc.run with time > 0
yogidevendra
force-pushed
the
APEXMALHAR-2195-LineReaderContext-last-record
branch
from
accc093
to
05e7f5c
Compare
|
@gauravgopi123 Incorporated your review comments. Could you please have a look? |
| while (posInStr < strBuffer.length()) { | ||
| char c = strBuffer.charAt(posInStr); | ||
| while (posInStr < bufferStr.length()) { | ||
| char c = bufferStr.charAt(posInStr); | ||
| if (c != '\r' && c != '\n') { | ||
| tmpBuilder.write(c); |
There was a problem hiding this comment.
This is lossy write to tmpBuilder using ByteArrayOutputStream.write(int). byte[] to String conversion (line 193) is based on default encoding. In case default encoding where multiple bytes are required to store chars, this will lead to lose of data...
After looking at the code little more, I also found that code can be simplified a lot and we can get rid of tmpBuilder and emptyBuilder
There was a problem hiding this comment.
@gauravgopi123 How about creating separate JIRA for addressing this issue?
We can have in-depth discussion on the issues, alternatives, possible solutions on the new JIRA.
There was a problem hiding this comment.
@yogidevendra What does it take to address the tmpBuilder issue now? Does not seem to require elaborate discussion. I would also prefer to get it fixed but if you are not able to work on it then take it up for next release.
There was a problem hiding this comment.
@tweise I will prefer to work on it later. Also, I have some possible alternative in my mind which I will propose in the new JIRA.
I have created new JIRA APEXMALHAR-2198 for this.
|
@gauravgopi123 @amberarrow any comments? |
|
@yogidevendra It'll probably be another week before I can look at this, swamped currently. |
|
@yogidevendra : Any reason to rush for this fix? If you plan to use LineReaderContext with only your fix, it will still be buggy. So I suggest that you fix the other issue and merge with this PR. |
|
@gauravgopi123 here are some reasons:
@amberarrow @tweise Any opinions on separate JIRA? |
| @@ -89,15 +89,18 @@ public void testDelimitedRecords() throws Exception | |||
| lma.prepareDAG(app, conf); | |||
| LocalMode.Controller lc = lma.getController(); | |||
| lc.setHeartbeatMonitoringEnabled(true); | |||
| lc.runAsync(); | |||
| lc.run(10 * 1000); | |||
There was a problem hiding this comment.
Please implement proper test termination. This topic was discussed elsewhere before.
There was a problem hiding this comment.
@tweise updated to handle proper test termination.
yogidevendra
force-pushed
the
APEXMALHAR-2195-LineReaderContext-last-record
branch
from
05e7f5c
to
b708b89
Compare
|
Your branch is behind by 18 commits. Please run |
yogidevendra
changed the title
yogidevendra
force-pushed
the
APEXMALHAR-2195-LineReaderContext-last-record
branch
from
b708b89
to
6ace0b2
Compare
|
@yogidevendra it looks like your upstream repo is the github mirror and not the asf repo. That would explain why there is the last commit missing. In the future please pull from the asf repo as sometimes sync to the mirror is delayed (saw it happening twice since yesterday). |
yogidevendra
force-pushed
the
APEXMALHAR-2195-LineReaderContext-last-record
branch
from
6ace0b2
to
c0af266
Compare
|
@tweise updated after rebase with asf. But, I noticed travis build is in pending state for long time for last few builds. Any clues? |
|
Yes, Travis builds also take really long. Not sure why. |
