Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly set default value to "UNFINISHED" for TraceBuilder's ResultType #349

Merged
merged 4 commits into from
Mar 25, 2024

Conversation

junchuanwang
Copy link
Contributor

@junchuanwang junchuanwang commented Mar 25, 2024

Summary

Symptom

There has been a report that some race condition can cause resultType being referenced as required constructor argument from a builder (during trace build) before it is being set.

StackTrace is as below:

java.lang.NullPointerException: resultType must not be null"java.lang.NullPointerException: resultType must not be null
  at com.linkedin.parseq.internal.ArgumentUtil.requireNotNull(ArgumentUtil.java:28)
  at com.linkedin.parseq.trace.ShallowTraceImp.<init>(ShallowTraceImp.java:52)
  at com.linkedin.parseq.trace.ShallowTraceBuilder.build(ShallowTraceBuilder.java:304)
  at com.linkedin.parseq.trace.TraceBuilder.lambda$build$1(TraceBuilder.java:85)
  at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1133)
  at com.linkedin.parseq.trace.TraceBuilder.build(TraceBuilder.java:85)
  at com.linkedin.parseq.BaseTask.getTrace(BaseTask.java:292)
  at com.linkedin.parseq.trace.TraceUtil.getJsonTrace(TraceUtil.java:22)
  at com.linkedin.parseq.ParSeqUnitTestHelper.logTracingResults(ParSeqUnitTestHelper.java:346)
  at com.linkedin.parseq.ParSeqUnitTestHelper.runAndWait(ParSeqUnitTestHelper.java:172)
  at com.linkedin.parseq.ParSeqUnitTestHelper.runAndWait(ParSeqUnitTestHelper.java:147)
  at com.linkedin.parseq.AbstractBaseEngineTest.runAndWait(AbstractBaseEngineTest.java:59)

Invesetigation

It is intermittent so it is rather hard to reproduce. By inspecting the code, it is also not clear when setResultType was be exempted from being called sometimes.

Impact assessment

The issue should only impact trace and we don't get such report for years. Only two users ever reported and both claim to be intermittent. I think we can set a default value of "UNFINISHED" so the exception won't be thrown during the run time. We therefore let the "bad" trace to be surfaced and investigated if the trace itself does not make sense.

Details about code change

  • Set an explicit default value of ResultType.UNFINISHED
  • Also fix another location where invalid ascii character were used and cannot build on certain machine (e.g. my machine which uses CBL-Mariner distribution)

Testing done

./gradlew build and ./gradlew test

@junchuanwang junchuanwang changed the title Juncwang/add defualt value for trade resulttype Explicitly set defualt value for Trace Result Type Mar 25, 2024
@junchuanwang junchuanwang changed the title Explicitly set defualt value for Trace Result Type Explicitly set defualt value for TraceBuilder's ResultType Mar 25, 2024
@junchuanwang junchuanwang changed the title Explicitly set defualt value for TraceBuilder's ResultType Explicitly set default value for TraceBuilder's ResultType Mar 25, 2024
@junchuanwang junchuanwang changed the title Explicitly set default value for TraceBuilder's ResultType Explicitly set default value to "UNFINISHED" for TraceBuilder's ResultType Mar 25, 2024
@junchuanwang junchuanwang merged commit a2ab8d8 into master Mar 25, 2024
1 check passed
@junchuanwang junchuanwang deleted the juncwang/add_defualt_value_for_trade_resulttype branch March 25, 2024 18:21
Copy link
Contributor

@mchen07 mchen07 left a comment

Choose a reason for hiding this comment

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

Thanks @junchuanwang for the quick fix.

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.

3 participants