search

brooklyncentral / brooklyn

This project has moved and is now part of the ASF
https://github.com/apache/incubator-brooklyn
72 stars 27 forks source link

Adds RebindExceptionHandler #1400

Closed aledsage closed 10 years ago

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2296 SUCCESS This pull request looks good (what's this?)

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2297 SUCCESS This pull request looks good (what's this?)

grkvlt commented 10 years ago

@aledsage A few things to look at, also a couple of questions:

  1. Did you imagine there would be different implementations of the RebindExceptionHandler eventually?
  2. Will there be a configuration key or command line option to set the FAIL_FAST etc rebind options?
aledsage commented 10 years ago

Thanks @grkvlt - I've incorporated those comments, except for moving more if the code inside the catch block.

For the far-too-many-maps-of-exceptions in RebindExceptionHandlerImpl, agree that code was ugly. However it was useful for assertions in tests. I've therefore simplified RebindExceptionHandlerImpl, but in a new RecordingRebindExceptionHandler extends RebindExceptionHandlerImpl it records the exception in a map and then calls super.

I imagine that we'll eventually have different RebindExceptionHandler implementations - but not certain. For example, there could be one for "interactive mode" that allows the user to try to fix the problems as they are encountered - but I suspect that wouldn't work yet as the methods can't tell the caller to retry the createEntity or the rebindEntity.

Yes, config key or CLI for setting fail_fast etc. I view this as a PR laying the ground work, rather than doing everything in one go. It really just preserves existing behavour but in a way that is more extensible. Subsequent PRs should make these options configurable.

I've also added @Beta to the RebindExceptionHandler interface.

I'm going to merge now, but happy to make more changes based on further feedback/discussion.

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2346 SUCCESS This pull request looks good (what's this?)