[BEAM-53] PubsubApiaryClient, PubsubTestClient #213
Conversation
|
R: @dhalperi |
Wrap Exceptions thrown in StartBundle in the InProcessPipelineRunner
|
I'm guessing this one fell off your radar @dhalperi. |
| LOG.error("Failed to delete subscription: ", e); | ||
| } | ||
| if (finallyBlockException != null) { | ||
| Throwables.propagate(finallyBlockException); |
There was a problem hiding this comment.
@kennknowles fyi
I think the current recommendation is simply:
throw new RuntimeException("Some message", finallyBlockException);Even if you intended to use Throwables.propagate, you should use throw Throwables.propagate... to work around some IDEs/cases.
|
May as well dump it here. To bootstrap review, can you please update the PR message with everything that's in the diff, assuming I didn't look at your original #120 and discussion with Kenn? |
| options.getGcpCredential(), | ||
| // Do not log 404. It clutters the output and is possibly even required by the caller. | ||
| new RetryHttpRequestInitializer(ImmutableList.of(404)))) | ||
| .setRootUrl(options.getPubsubRootUrl()) |
There was a problem hiding this comment.
Do you need to do any pubsub-specific failure handling here? 410, 403 rate limited, any of the crap we see in other APIs?
There was a problem hiding this comment.
This is straight from the impl we already have in Transport. It has been load tested, used both from vms and desktop, and seems to do the business.
Note that I'd like to remove Transport.newPubsubClient but it is used from various examples so leaving it in place for a future refactor. I at least marked ti deprecated.
mshields822
changed the title
|
(off for root canal. forgive any grumpiness later today...) |
|
|
||
| public String getV1Beta1Path() { | ||
| String[] splits = path.split("/"); | ||
| Preconditions.checkState(splits.length == 4); |
There was a problem hiding this comment.
In general, would strongly prefer to have better error messages at all of these validation checks. For example,
checkState(splits.length == 4, "Expected 4 path components in Pubsub path %s", path);
|
LGTM modulo a pending rebase and few minor (optional) comments. |
| // Otherwise we'll fall back to the topic's project. | ||
| // Note that they don't need to be the same. | ||
| String project = c.getPipelineOptions().as(PubsubOptions.class).getProject(); | ||
| if (project == null || project.isEmpty()) { |
|
PTAL |
3 participants
Continuing from #120