Github user DarinJ commented on a diff in the pull request:

    https://github.com/apache/incubator-pirk/pull/93#discussion_r80026380
  
    --- Diff: 
src/main/java/org/apache/pirk/responder/wideskies/ResponderDriver.java ---
    @@ -50,103 +40,31 @@
     {
       private static final Logger logger = 
LoggerFactory.getLogger(ResponderDriver.class);
     
    -  private enum Platform
    -  {
    -    MAPREDUCE, SPARK, SPARKSTREAMING, STORM, STANDALONE, NONE
    -  }
    -
    -  public static void main(String[] args) throws Exception
    +  public static void main(String[] args)
       {
         ResponderCLI responderCLI = new ResponderCLI(args);
     
    -    // For handling System.exit calls from Spark Streaming
    -    System.setSecurityManager(new SystemExitManager());
    -
    -    Platform platform = Platform.NONE;
    -    String platformString = 
SystemConfiguration.getProperty(ResponderProps.PLATFORM);
    +    String platformName = 
SystemConfiguration.getProperty(ResponderProps.PLATFORM, "None");
    +    logger.info("Attempting to use platform {} ...", platformName);
         try
         {
    -      platform = Platform.valueOf(platformString.toUpperCase());
    -    } catch (IllegalArgumentException e)
    -    {
    -      logger.error("platform " + platformString + " not found.");
    -    }
    -
    -    logger.info("platform = " + platform);
    -    switch (platform)
    -    {
    -      case MAPREDUCE:
    -        logger.info("Launching MapReduce ResponderTool:");
    -
    -        ComputeResponseTool pirWLTool = new ComputeResponseTool();
    -        ToolRunner.run(pirWLTool, new String[] {});
    -        break;
    -
    -      case SPARK:
    -        logger.info("Launching Spark ComputeResponse:");
    -
    -        ComputeResponse computeResponse = new 
ComputeResponse(FileSystem.get(new Configuration()));
    -        computeResponse.performQuery();
    -        break;
    -
    -      case SPARKSTREAMING:
    -        logger.info("Launching Spark ComputeStreamingResponse:");
    -
    -        ComputeStreamingResponse computeSR = new 
ComputeStreamingResponse(FileSystem.get(new Configuration()));
    -        try
    -        {
    -          computeSR.performQuery();
    -        } catch (SystemExitException e)
    -        {
    -          // If System.exit(0) is not caught from Spark Streaming,
    -          // the application will complete with a 'failed' status
    -          logger.info("Exited with System.exit(0) from Spark Streaming");
    -        }
    -
    -        // Teardown the context
    -        computeSR.teardown();
    -        break;
    -
    -      case STORM:
    -        logger.info("Launching Storm PirkTopology:");
    -        PirkTopology.runPirkTopology();
    -        break;
    -
    -      case STANDALONE:
    -        logger.info("Launching Standalone Responder:");
    -
    -        String queryInput = 
SystemConfiguration.getProperty("pir.queryInput");
    -        Query query = new LocalFileSystemStore().recall(queryInput, 
Query.class);
    -
    -        Responder pirResponder = new Responder(query);
    -        pirResponder.computeStandaloneResponse();
    -        break;
    -    }
    -  }
    -
    -  // Exception and Security Manager classes used to catch System.exit from 
Spark Streaming
    -  private static class SystemExitException extends SecurityException
    -  {}
    -
    -  private static class SystemExitManager extends SecurityManager
    -  {
    -    @Override
    -    public void checkPermission(Permission perm)
    -    {}
    -
    -    @Override
    -    public void checkExit(int status)
    -    {
    -      super.checkExit(status);
    -      if (status == 0) // If we exited cleanly, throw SystemExitException
    +      ResponderPlugin responder = 
ResponderService.getInstance().getResponder(platformName);
    +      if (responder == null)
           {
    -        throw new SystemExitException();
    +        logger.error("No such platform plugin found: {}!", platformName);
           }
           else
           {
    -        throw new SecurityException();
    +        responder.run();
           }
    -
    +    }
    +    catch (PIRException pirEx)
    --- End diff --
    
    Hmm, yeah I didn't like that Exception was being caught and turned to 
PirException and I lost the stacktrace.  But yeah it's really unnecessary.
    
    I'm frustrated with the Exception issue as well and It seems like there's a 
lot of discussion about exceptions right now which I was trying to code around. 
 I don't want to force opinions on this in a modest PR, if you've got a plan 
can you point me to it and I'll follow that pattern and trust it'll get cleaned 
up. 
    
    I think in the current state PIRException by itself can hide a lot of 
exceptions if we just catch Exception to throw PirException.  I've looked over 
a lot of the exceptions we're currently throwing, many are just illegal 
argument or illegal state, a lot could be done with Guava's Preconditions or a 
class similar to it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to