[POC] [AbfsInputStream] Add position and reader type in tracing context #129
Conversation
There was a problem hiding this comment.
Great code!
readRemote does create new tracingContext object before going to client.
Since, we are adding properties in realtime on singleton tracingContext, I feel corruption can happen. What if on the given FS op on inputStream, we create child tracingContext before adding any property to it.
| } | ||
|
|
||
| public void sendReadMetrics(TracingContext tracingContext) throws IOException { | ||
| if (!abfsConfiguration.isReadCallsMetricEnabled() || readMetricData.length() == 0) { |
There was a problem hiding this comment.
Since readMetricData is init with some string, length should not be 0. i think we can remove it.
| createReadMetircFile(readMetricFileName, tracingContext1); | ||
| appendToReadMetricFile(readMetricFileName, tracingContext1); | ||
| flushReadMetricFile(readMetricFileName, tracingContext1); |
There was a problem hiding this comment.
Since we are at client level, what if we directly call createPath, append, flush.
| @@ -562,6 +575,8 @@ int readRemote(long position, byte[] b, int offset, int length, TracingContext t | |||
| streamStatistics.remoteReadOperation(); | |||
| } | |||
| LOG.trace("Trigger client.read for path={} position={} offset={} length={}", path, position, offset, length); | |||
| tracingContext.getHeader(); | |||
There was a problem hiding this comment.
hope its not required.
| @@ -211,6 +212,8 @@ public int read(long position, byte[] buffer, int offset, int length) | |||
| if (streamStatistics != null) { | |||
| streamStatistics.readOperationStarted(); | |||
| } | |||
| LOG.debug("Direct read with position called, no Optimizations"); | |||
| tracingContext.setReaderID("NR"); | |||
There was a problem hiding this comment.
we should have a new tracingContext object in the method, else parallel thread would use same tc and probably metric-corruption can happen.
| if (firstRead && n > 0) { | ||
| tracingContext.setFirstReadPosition(AbfsClient.getNormalizedValue(n, 4 * ONE_MB)); | ||
| tracingContext.setFirstReadPositionFromEnd(AbfsClient.getNormalizedValue(contentLength - n, 4 * ONE_MB)); | ||
| } else { | ||
| tracingContext.setFirstReadPosition(""); |
There was a problem hiding this comment.
this might reflect on all the calls post seek. Any way if we can reset it.
| } finally { | ||
| this.isSendMetricCall = false; | ||
| } | ||
| } | ||
|
|
||
| public void updateReadMetrics(String inputStreamId, int bufferLength, int requestedLength, long contentLength, long nextReadPos, boolean firstRead) { |
There was a problem hiding this comment.
lets have it synchronized.
anujmodi2021
changed the title
Tracing header will have 2 more things:
Note some tracing validator tests are failing but functional tests are working fine and header is constructed as expected.