make buildSegmentInternal raise error out explicitly by walterddr · Pull Request #7896 · apache/pinot

walterddr · 2021-12-13T16:13:42Z

this fixes the issue when datamanager doesn't log segment error properly and shown to user. stacktrace basically only returns a null value and not useful for debugging.

this fixes the issue when datamanager doesn't log segment error properly and shown to user

codecov-commenter · 2021-12-13T19:16:05Z

Codecov Report

Merging #7896 (1b38345) into master (fb12a50) will decrease coverage by 6.42%.
The diff coverage is 7.69%.

@@             Coverage Diff              @@
##             master    #7896      +/-   ##
============================================
- Coverage     71.34%   64.91%   -6.43%     
  Complexity     4087     4087              
============================================
  Files          1587     1544      -43     
  Lines         82071    80222    -1849     
  Branches      12267    12062     -205     
============================================
- Hits          58553    52080    -6473     
- Misses        19550    24417    +4867     
+ Partials       3968     3725     -243

Flag	Coverage Δ
integration1	`?`
integration2	`?`
unittests1	`68.30% <7.69%> (-0.03%)`	⬇️
unittests2	`14.38% <0.00%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...manager/realtime/LLRealtimeSegmentDataManager.java	`47.81% <7.69%> (-24.95%)`	⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java	`0.00% <0.00%> (-100.00%)`	⬇️
...g/apache/pinot/common/metrics/ControllerMeter.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../apache/pinot/common/metrics/BrokerQueryPhase.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../apache/pinot/common/metrics/MinionQueryPhase.java	`0.00% <0.00%> (-100.00%)`	⬇️
...he/pinot/common/messages/SegmentReloadMessage.java	`0.00% <0.00%> (-100.00%)`	⬇️
...pinot/core/data/manager/realtime/TimerService.java	`0.00% <0.00%> (-100.00%)`	⬇️
...pinot/minion/exception/TaskCancelledException.java	`0.00% <0.00%> (-100.00%)`	⬇️
...not/common/exception/HttpErrorStatusException.java	`0.00% <0.00%> (-100.00%)`	⬇️
...t/core/startree/plan/StarTreeDocIdSetPlanNode.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 371 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb12a50...1b38345. Read the comment docs.

Jackie-Jiang

We need a bigger refactoring and make all methods propagating the error through the exception

Jackie-Jiang · 2021-12-14T01:52:45Z

.../src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java

  }

-  protected boolean buildSegmentAndReplace() {
+  protected void buildSegmentAndReplace() throws Exception {


(code format) Please setup the code format following: https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#setup-ide

Jackie-Jiang · 2021-12-14T01:52:56Z

.../test/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManagerTest.java


    @Override
-    protected boolean buildSegmentAndReplace() {
+    protected void buildSegmentAndReplace() throws Exception {


(code format) same

Jackie-Jiang · 2021-12-14T01:54:57Z

.../src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java


-  protected boolean buildSegmentAndReplace() {
+  protected void buildSegmentAndReplace() throws Exception {
    SegmentBuildDescriptor descriptor = buildSegmentInternal(false);


(major) when segment build failed, it will return null descriptor instead of throwing exception. In order to make it work, we need to change buildSegmentInternal() to throw exception as well

yes. buildSegmentInternal has already been changed to throw exception.

Sorry I missed that. When we throw the exception, we should not log the error. The catcher is responsible for logging the error, or we will log multiple errors for the same exception

yup. working on it.

sajjad-moradi · 2021-12-14T06:03:34Z

.../src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java

-                  success = buildSegmentAndReplace();
-                  if (success) {
+                  try {
+                    buildSegmentAndReplace();


This is not the only place buildSegmentAndReplace is being used. It's also being used two times in goOnlineFromConsuming method. If we throw exception in buildSegmentAndRepalce, then the segment goes to error state in external view upon receiving helix transition message consuming -> online.
The existing behaviour is even worse because segment is not built but external view will move to online state!!
I suggest we proceed with throwing exception in buildSegmentAndReplace, but we try-catch its usage in goOnlineFromConsuming and in the catch part, we use downloadSegmentAndReplace.
@mcvsubbu @Jackie-Jiang what do you guys think?

yes I thought about exactly the same thing. the return null also make the caller alter the state --> if we do not try catch it properly the states wont change.

Ultimately I think we need to rethink when should _realtimeTableDataManager.addSegmentError occur, my opinion is that we should make a single state machine function for each functionality, in this case: the only places we catch and process exception should be: PartitionConsumer and goOnlineFromConsuming, in which both modifies the internal state. the rest of the internal methods should all bubble up the exception by catch and rethrowing.

adjusted plz kindly take a look

The state should be set to DISCARDED if buildSegmentAndReplace fails for whatever reason (It really should not, since another host was able to build). This will allow us to replace the segment by downloading, as a last effort to keep the service going.

What exception are you seeing? Can you paste a stack?

First of all, this is not an addSegment error (unless that method in RealtimeTableDataManager has been misnamed). Secondly, this is an "error" that is totally recoverable, since some other host has already completed the segment, and it can be downloaded. So, no "error" should be raised in this condition.

If you can please specify the problem you are trying to solve, we can help.

OK, I realized that the addSegmentError is a method that adds all segment errors. But my question still stands. When we detect a recoverable error, do we still want to add an error? I don't think so. I think we should call addSegmentError() API only when the error is unrecoverable.

…te changes

sajjad-moradi · 2021-12-14T19:50:44Z

.../src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java

-              .error("Failed to find file: {} under index directory: {}", V1Constants.MetadataKeys.METADATA_FILE_NAME,
-                  indexDir);
-          return null;
+          throw new IllegalStateException(String.format("Failed to find file: %s under index directory: %s",


IllegalStateException doesn't get caught in goOnlineFromConsuming so the desired segment download step wouldn't happen in this case.

mcvsubbu · 2021-12-15T00:02:40Z

@walterddr is there an issue created that has some details on what really happened? Or else, if you can describe what the problem was, that will help as well (maybe it was discussed in the chat channel, but it would help to have it in this PR). Thanks

mcvsubbu · 2021-12-15T00:08:22Z

.../src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java

-                  success = buildSegmentAndReplace();
-                  if (success) {
+                  try {
+                    buildSegmentAndReplace();


The state should be set to DISCARDED if buildSegmentAndReplace fails for whatever reason (It really should not, since another host was able to build). This will allow us to replace the segment by downloading, as a last effort to keep the service going.

What exception are you seeing? Can you paste a stack?

mcvsubbu · 2021-12-15T00:08:42Z

.../src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java

                    _realtimeTableDataManager.addSegmentError(_segmentNameStr,
-                        new SegmentErrorInfo(System.currentTimeMillis(), "Could not build segment", null));
+                        new SegmentErrorInfo(System.currentTimeMillis(), "Could not build segment!", e));
+                    _state = State.ERROR;


see comment above regarding state

walterddr · 2021-12-15T03:39:04Z

@walterddr is there an issue created that has some details on what really happened? Or else, if you can describe what the problem was, that will help as well (maybe it was discussed in the chat channel, but it would help to have it in this PR). Thanks

this was actually a very great question @mcvsubbu .
so initially the PR was only to address the fact that 2 of the place where _realtimeTableDataManager.addSegmentError only logs a single string "Could not build segment", with no stack trace attached.
I later discover that this is because the several protected functions that returns "null" indicating the segment creation fail doesn't bubble up the stack trace therefore there's no stacktrace to report.

Thanks for calling this out I think we deviated from the original purpose of this PR. I think we are now addressing a related but different (and bigger) issue.

I can create an issue for what's being discussed here. But for the null-stacktrace issue we can simple move the reporting to where the actual exception was thrown. what do you guys think? (@sajjad-moradi @Jackie-Jiang )

sajjad-moradi · 2021-12-15T06:23:26Z

I can create an issue for what's being discussed here. But for the null-stacktrace issue we can simple move the reporting to where the actual exception was thrown. what do you guys think? (@sajjad-moradi @Jackie-Jiang )

That's a good idea. It makes the changes in this PR small. We can create a separate PR for the other issue.

walterddr · 2021-12-15T15:42:38Z

sounds good. i will go ahead and summarize and create the issue; meanwhile I created a simpler PR to address the null stacktrace here: #7909

mcvsubbu · 2021-12-15T17:07:10Z

I still don't understand the problem that was faced in the installation (or, is this something you noticed in the source and want to fix)? The reason I ask is that there are many things going on when the segment finishes, and changing the behavior around that time needs to be done carefully.

Assuming this was a problem in some installation, can you elaborate the issue? (e.g. you noticed segment was not consuming, you looked at the debug api, but found null stack traces, you looked at the server logs and found the segment build had failed due to X -- put down the reason please). Thanks! And if you found a stack trace in the server that you would like to see in debug API, please add that as well. We need to know if the stack trace was while transitioning from consuming to online, or whether it was before that during segment completion protocol.

thanks for your patience.

walterddr · 2021-12-15T18:39:46Z

closing this. please move to issue for discussion.

make buildSegmentInternal raise error out explicitly

5aebcb1

this fixes the issue when datamanager doesn't log segment error properly and shown to user

walterddr force-pushed the fix_rtsegmanager_error branch from af76bec to 5aebcb1 Compare December 13, 2021 18:42

Jackie-Jiang reviewed Dec 14, 2021

View reviewed changes

address diff comments

1b38345

sajjad-moradi reviewed Dec 14, 2021

View reviewed changes

added SegmentGenerationException and specific try-catch to handle sta…

f13bcd4

…te changes

sajjad-moradi reviewed Dec 14, 2021

View reviewed changes

mcvsubbu reviewed Dec 15, 2021

View reviewed changes

walterddr mentioned this pull request Dec 15, 2021

Improve segment-level DebugInfo #7910

Open

walterddr closed this Dec 15, 2021

walterddr deleted the fix_rtsegmanager_error branch December 6, 2023 16:19

Conversation

walterddr commented Dec 13, 2021

Uh oh!

codecov-commenter commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sajjad-moradi Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterddr Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcvsubbu commented Dec 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterddr commented Dec 15, 2021

Uh oh!

sajjad-moradi commented Dec 15, 2021

Uh oh!

walterddr commented Dec 15, 2021

Uh oh!

mcvsubbu commented Dec 15, 2021

Uh oh!

walterddr commented Dec 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Dec 13, 2021 •

edited

Loading

sajjad-moradi Dec 14, 2021 •

edited

Loading

walterddr Dec 14, 2021 •

edited

Loading