Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2743#discussion_r198988037
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/timed/TimedResource.java ---
@@ -0,0 +1,29 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements.
+ * See the NOTICE file distributed with this work for additional
information regarding copyright ownership.
+ * The ASF 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 org.apache.storm.metric.timed;
+
+import com.codahale.metrics.Timer;
+
+public class TimedResource<T extends AutoCloseable> extends Timed<T> {
+ public TimedResource(T measured, Timer timer) {
+ super(measured, timer);
+ }
+
+ @Override
+ public void close() throws Exception {
+ super.close();
--- End diff --
Could we please change this a bit? Stopping the timer is not nearly as
important as actually closing the resource, and if stopping the timer throws
any kind of exception it will not stop the resource. Could we do something
more like.
```
try {
super.close();
} finally {
getMeasured().close();
}
```
This should guarantee that we close the resource always, and if an
exception is thrown in both cases the resource exception takes precedent and in
newer versions of java the other exception will show up as a Suppressed
exception.
---
