[GitHub] [pinot] sajjad-moradi commented on a change in pull request #7896: make buildSegmentInternal raise error out explicitly

[GitBox <gi...@apache.org>]

sajjad-moradi commented on a change in pull request #7896:
URL: https://github.com/apache/pinot/pull/7896#discussion_r768340557



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java
##########
@@ -624,14 +624,14 @@ public void run() {
                   _state = State.DISCARDED;
                   break;
                 case DEFAULT:
-                  success = buildSegmentAndReplace();
-                  if (success) {
+                  try {
+                    buildSegmentAndReplace();

Review comment:
       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 -> offline. 
   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?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org