-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[QTL] Query time lookup cluster wide config #1576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * Licensed to Metamarkets Group Inc. (Metamarkets) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. Metamarkets licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package io.druid.common.utils; | ||
|
|
||
| import com.google.common.collect.ImmutableMap; | ||
|
|
||
| import javax.annotation.Nullable; | ||
| import javax.validation.constraints.NotNull; | ||
| import java.util.Map; | ||
|
|
||
| public class ServletResourceUtils | ||
| { | ||
| /** | ||
| * Sanitize the exception as a map of "error" to information about the exception. | ||
| * | ||
| * This method explicitly suppresses the stack trace and any other logging. Any logging should be handled by the caller. | ||
| * @param t The exception to sanitize | ||
| * @return An immutable Map with a single entry which maps "error" to information about the error suitable for passing as an entity in a servlet error response. | ||
| */ | ||
| public static Map<String, String> sanitizeException(@Nullable Throwable t) | ||
| { | ||
| return ImmutableMap.of( | ||
| "error", | ||
| t == null ? "null" : (t.getMessage() == null ? t.toString() : t.getMessage()) | ||
| ); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| /* | ||
| * Licensed to Metamarkets Group Inc. (Metamarkets) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. Metamarkets licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package io.druid.common.utils; | ||
|
|
||
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
|
|
||
| public class ServletResourceUtilsTest | ||
| { | ||
|
|
||
| @Test | ||
| public void testSanitizeException() throws Exception | ||
| { | ||
| final String message = "some message"; | ||
| Assert.assertEquals(message, ServletResourceUtils.sanitizeException(new Throwable(message)).get("error")); | ||
| Assert.assertEquals("null", ServletResourceUtils.sanitizeException(null).get("error")); | ||
| Assert.assertEquals(message, ServletResourceUtils.sanitizeException(new Throwable() | ||
| { | ||
| @Override | ||
| public String toString() | ||
| { | ||
| return message; | ||
| } | ||
| }).get("error")); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,3 +102,16 @@ To view last <n> entries of the audit history of coordinator dynamic config issu | |
| ``` | ||
| http://<COORDINATOR_IP>:<PORT>/druid/coordinator/v1/config/history?count=<n> | ||
| ``` | ||
|
|
||
|
|
||
| # Lookups Dynamic Config (EXPERIMENTAL) | ||
| These configuration options control the behavior of the Lookup dynamic configuration described in the [lookups page](../querying/lookups.html) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we mention that this is coordinator config. it is not big deal
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, it's in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
| |Property|Description|Default| | ||
| |--------|-----------|-------| | ||
| |`druid.manager.lookups.hostDeleteTimeout`|How long to wait for a `DELETE` request to a particular node before considering the `DELETE` a failure|PT1s| | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it makes more sense to call it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made it |
||
| |`druid.manager.lookups.hostUpdateTimeout`|How long to wait for a `POST` request to a particular node before considering the `POST` a failure|PT10s| | ||
| |`druid.manager.lookups.deleteAllTimeout`|How long to wait for all `DELETE` requests to finish before considering the delete attempt a failure|PT10s| | ||
| |`druid.manager.lookups.updateAllTimeout`|How long to wait for all `POST` requests to finish before considering the attempt a failure|PT60s| | ||
| |`druid.manager.lookups.threadPoolSize`|How many nodes can be managed concurrently (concurrent POST and DELETE requests). Requests this limit will wait in a queue until a slot becomes available.|10| | ||
| |`druid.manager.lookups.period`|How many milliseconds between checks for configuration changes|30_000| | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -302,3 +302,262 @@ To test this setup, you can send key/value pairs to a kafka stream via the follo | |
| ``` | ||
|
|
||
| Renames can then be published as `OLD_VAL->NEW_VAL` followed by newline (enter or return) | ||
|
|
||
| Dynamic configuration (EXPERIMENTAL) | ||
| ------------------------------------ | ||
|
|
||
| The following documents the behavior of the cluster-wide config which is accessible through the coordinator. | ||
| The configuration is propagated through the concept of "tier" of servers. | ||
| A "tier" is defined as a group of services which should receive a set of lookups. | ||
| For example, you might have all historicals be part of `__default`, and Peons be part of individual tiers for the datasources they are tasked with. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a note to make sure that ppls understand that historical tiers is independent from this tier.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| The tiers for lookups are completely independent of historical tiers. | ||
|
|
||
| These configs are accessed using JSON through the following URI template | ||
|
|
||
| ``` | ||
| http://<COORDINATOR_IP>:<PORT>/druid/coordinator/v1/lookups/{tier}/{id} | ||
| ``` | ||
|
|
||
| All URIs below are assumed to have `http://<COORDINATOR_IP>:<PORT>` prepended. | ||
|
|
||
| If you have NEVER configured lookups before, you MUST post an empty json object `{}` to `/druid/coordinator/v1/lookups` to initialize the configuration. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we avoid this ? if not, it is not a big of a deal thought
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern is that if the connection to the DB is temporarily down when a node starts, it is possible that the local node has no knowledge of configuration. If we take an "update" object, and allow it to override a non-existent config, it is not clear to me that the old config would be preserved whenever the DB comes back up. Having this is kind of a "safety check" to make sure that you really are disregarding whatever may have been in the config previously. Per cluster it should only ever have to be done ONCE, and after that any occurrence of an empty config in the coordinator is an error.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but this is not intuitive for user, can we make sure that when the coordinator starts it does what it needs to make it working, instead of relying on users. Also what happen if this query fails ? how do you deal with that ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having an original initialization step seems perfectly reasonable to me, and the instructions for what to do are both in the docs and as part of the error message in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IF the coordinator is connected to the database, then any failures will be bubbled up as a 500
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If i understand this correctly, there is already an initialization step in coordinator for creating default rule quite similar to this. This seems like the same step where we can create defaults when initializing the db ? |
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add docs about get tiers, it is important for druid user to list all the active tiers.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great ! |
||
| These endpoints will return one of the following results: | ||
|
|
||
| * 404 if the resource is not found | ||
| * 400 if there is a problem in the formatting of the request | ||
| * 202 if the request was accepted asynchronously (`POST` and `DELETE`) | ||
| * 200 if the request succeeded (`GET` only) | ||
|
|
||
| ## Configuration propagation behavior | ||
| The configuration is propagated to the query serving nodes (broker / router / peon / historical) by the coordinator. | ||
| The query serving nodes have an internal API for managing `POST`/`GET`/`DELETE` of lookups. | ||
| The coordinator periodically checks the dynamic configuration for changes and, when it detects a change it does the following: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to mention to which period we refer to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding timing configs last in this PR because adding config options is small compared to logic changes. Once logic stuff is solidified I'll add config.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, i do add those comment as i am reading like that we don't forget about it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added configs, please check latest commit diff for config delta |
||
|
|
||
| 1. Post all lookups for a tier to all Druid nodes within that tier. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we explain the cases when add return exception from some nodes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated, let me know if the added explanation helps. |
||
| 2. Delete lookups from a tier which were dropped between the prior configuration values and this one. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i am not sure this reflect the new strategy ? i guess it is better to delete upon request ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The delete and management of the delete are asynchronous. The endpoints the coordinator expose simply modify the config. There is a background thread which manages configuration changes on a defined cadence. If a user issues a DELETE to the coordinator, that lookup will be removed from the config, and then at some point in the future, when the config propagator fires its management routine, it will figure out which ones were deleted, and issue the appropriate delete requests.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it will NOT try to delete things which are not part of the config. |
||
|
|
||
| If there is no configuration change, the coordinator checks for any nodes which might be new since the last time it propagated lookups and adds all lookups for that node (assuming that node's tier has lookups). | ||
| If there are errors while trying to add or update configuration on a node, that node is temporarily skipped until the next management period. The next management period the update will attempt to be propagated again. | ||
| If there is an error while trying to delete a lookup from a node (or if a node is down when the coordinator is propagating the config), the delete is not attempted again. In such a case it is possible that a node has lookups that are no longer managed by the coordinator. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to fix this by syncing up the states on the next management period ?,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That had been discussed as a kind of "dictator" mode, but is not in this MVP pr. Right now it is in a friendly mode where you can add lookups to a node manually which are not part of the dynamic config. Adding such a feature as you describe would make it enforce its view of state on all nodes instead of allowing other things to modify the node state. For this PR I went with not enforcing dictator mode.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool, I am fine with leaving it as it is.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. filed |
||
|
|
||
| ## Bulk update | ||
| Lookups can be updated in bulk by posting a JSON object to `/druid/coordinator/v1/lookups`. The format of the json object is as follows: | ||
|
|
||
|
|
||
| ```json | ||
| { | ||
| "tierName": { | ||
| "lookupExtractorFactoryName": { | ||
| "someExtractorField": "someExtractorValue" | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| So a config might look something like: | ||
| ```json | ||
| { | ||
| "__default": { | ||
| "country_code": { | ||
| "type": "simple_json", | ||
| "uri": "http://some.host.com/codes.json" | ||
| }, | ||
| "site_id": { | ||
| "type": "confidential_jdbc", | ||
| "auth": "/etc/jdbc.internal", | ||
| "table": "sites", | ||
| "key": "site_id", | ||
| "value": "site_name" | ||
| }, | ||
| "site_id_customer1": { | ||
| "type": "confidential_jdbc", | ||
| "auth": "/etc/jdbc.customer1", | ||
| "table": "sites", | ||
| "key": "site_id", | ||
| "value": "site_name" | ||
| }, | ||
| "site_id_customer2": { | ||
| "type": "confidential_jdbc", | ||
| "auth": "/etc/jdbc.customer2", | ||
| "table": "sites", | ||
| "key": "site_id", | ||
| "value": "site_name" | ||
| } | ||
| }, | ||
| "realtime_customer1": { | ||
| "country_code": { | ||
| "type": "simple_json", | ||
| "uri": "http://some.host.com/codes.json" | ||
| }, | ||
| "site_id_customer1": { | ||
| "type": "confidential_jdbc", | ||
| "auth": "/etc/jdbc.customer1", | ||
| "table": "sites", | ||
| "key": "site_id", | ||
| "value": "site_name" | ||
| } | ||
| }, | ||
| "realtime_customer2": { | ||
| "country_code": { | ||
| "type": "simple_json", | ||
| "uri": "http://some.host.com/codes.json" | ||
| }, | ||
| "site_id_customer2": { | ||
| "type": "confidential_jdbc", | ||
| "auth": "/etc/jdbc.customer2", | ||
| "table": "sites", | ||
| "key": "site_id", | ||
| "value": "site_name" | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| All entries in the map will UPDATE existing entries. No entries will be deleted. | ||
|
|
||
| ## Update Lookup | ||
| A `POST` to a particular lookup extractor factory via `/druid/coordinator/v1/lookups/{tier}/{id}` will update that specific extractor factory. | ||
|
|
||
| For example, a post to `/druid/coordinator/v1/lookups/realtime_customer1/site_id_customer1` might contain the following: | ||
|
|
||
| ```json | ||
| { | ||
| "type": "confidential_jdbc", | ||
| "auth": "/etc/jdbc.customer1", | ||
| "table": "sites_updated", | ||
| "key": "site_id", | ||
| "value": "site_name" | ||
| } | ||
| ``` | ||
|
|
||
| This will replace the `site_id_customer1` lookup in the `realtime_customer1` with the definition above. | ||
|
|
||
| ## Get Lookup | ||
| A `GET` to a particular lookup extractor factory is accomplished via `/druid/coordinator/v1/lookups/{tier}/{id}` | ||
|
|
||
| Using the prior example, a `GET` to `/druid/coordinator/v1/lookups/realtime_customer2/site_id_customer2` should return | ||
|
|
||
| ```json | ||
| { | ||
| "type": "confidential_jdbc", | ||
| "auth": "/etc/jdbc.customer2", | ||
| "table": "sites", | ||
| "key": "site_id", | ||
| "value": "site_name" | ||
| } | ||
| ``` | ||
|
|
||
| ## Delete Lookup | ||
| A `DELETE` to `/druid/coordinator/v1/lookups/{tier}/{id}` will remove that lookup from the cluster. | ||
|
|
||
| ## List tier names | ||
| A `GET` to `/druid/coordinator/v1/lookups` will return a list of known tier names in the dynamic configuration. | ||
| To discover a list of tiers currently active in the cluster **instead of** ones known in the dynamic configuration, the parameter `discover=true` can be added as per `/druid/coordinator/v1/lookups?discover=true`. | ||
|
|
||
| ## List lookup names | ||
| A `GET` to `/druid/coordinator/v1/lookups/{tier}` will return a list of known lookup names for that tier. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No |
||
|
|
||
| # Internal API | ||
|
|
||
| The Peon, Router, Broker, and Historical nodes all have the ability to consume lookup configuration. | ||
| There is an internal API these nodes use to list/load/drop their lookups starting at `/druid/listen/v1/lookups`. | ||
| These follow the same convention for return values as the cluster wide dynamic configuration. | ||
| Usage of these endpoints is quite advanced and not recommended for most users. | ||
| The endpoints are as follows: | ||
|
|
||
| ## Get Lookups | ||
|
|
||
| A `GET` to the node at `/druid/listen/v1/lookups` will return a json map of all the lookups currently active on the node. | ||
| The return value will be a json map of the lookups to their extractor factories. | ||
|
|
||
| ```json | ||
|
|
||
| { | ||
| "some_lookup_name": { | ||
| "type": "simple_json", | ||
| "uri": "http://some.host.com/codes.json" | ||
| } | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| ## Get Lookup | ||
|
|
||
| A `GET` to the node at `/druid/listen/v1/lookups/some_lookup_name` will return the LookupExtractorFactory for the lookup identified by `some_lookup_name`. | ||
| The return value will be the json representation of the factory. | ||
|
|
||
| ```json | ||
| { | ||
| "type": "simple_json", | ||
| "uri": "http://some.host.com/codes.json" | ||
| } | ||
| ``` | ||
|
|
||
| ## Bulk Add or Update Lookups | ||
|
|
||
| A `POST` to the node at `/druid/listen/v1/lookups` of a JSON map of lookup names to LookupExtractorFactory will cause the service to add or update its lookups. | ||
| The return value will be a JSON map in the following format: | ||
|
|
||
| ```json | ||
| { | ||
| "status": "accepted", | ||
| "failedUpdates": {} | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| If a lookup cannot be started, or is left in an undefined state, the lookup in error will be returned in the `failedUpdates` field as per: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but this will happen asynchronously, so the user can not see this, unless you search the logs ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to add an alert in the coordinator side.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is part of the coordinator it doesn't really fit under
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a function of the API when pinging a historical directly; it is part of the internal API |
||
|
|
||
| ```json | ||
| { | ||
| "status": "accepted", | ||
| "failedUpdates": { | ||
| "country_code": { | ||
| "type": "simple_json", | ||
| "uri": "http://some.host.com/codes.json" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| The `failedUpdates` field of the return value should be checked if a user is wanting to assure that every update succeeded. | ||
|
|
||
| ## Add or Update Lookup | ||
|
|
||
| A `POST` to the node at `/druid/listen/v1/lookups/some_lookup_name` will behave very similarly to a bulk update. | ||
|
|
||
| If `some_lookup_name` is desired to have the LookupExtractorFactory definition of | ||
|
|
||
| ```json | ||
| { | ||
| "type": "simple_json", | ||
| "uri": "http://some.host.com/codes.json" | ||
| } | ||
| ``` | ||
|
|
||
| Then a post to `/druid/listen/v1/lookups/some_lookup_name` will behave the same as a `POST` to `/druid/listen/v1/lookups` of | ||
|
|
||
| ```json | ||
|
|
||
| { | ||
| "some_lookup_name": { | ||
| "type": "simple_json", | ||
| "uri": "http://some.host.com/codes.json" | ||
| } | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| ## Remove a Lookup | ||
| A `DELETE` to `/druid/listen/v1/lookups/some_lookup_name` will remove that lookup from the node. Success will reflect the ID. | ||
|
|
||
| # Configuration | ||
| See the [coordinator configuration guilde](../configuration/coordinator.html) for coordinator configuration | ||
|
|
||
| To configure a Broker / Router / Historical / Peon to announce itself as part of a lookup tier, use the `druid.zk.paths.lookupTier` property. | ||
|
|
||
| |Property | Description | Default | | ||
| |---------|-------------|---------| | ||
| |`druid.lookup.tierName`| The tier for **lookups** for this node. This is independent of other tiers.|`__default`| | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how this is different ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the intended priority in the face of null values is:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry but how
is different from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A throwable's toString is not guaranteed to be equal to its message. It can sometimes have more information. See
org.jets3t.service.ServiceException#toStringfor an example.At a minimum, if a throwable class doesn't implement a toString, and doesn't have a message, then it should at least print out something like
SomeCrazyException@78a31dI could have tested the throwable's toString before the call, and use that as the expected value. But I thought it was easier in this test to explicitly specify the toString result, and verify against that.