[GitHub] [hadoop] mukund-thakur commented on a change in pull request #1820: HADOOP-16830. Add public IOStatistics API + S3A implementation
mukund-thakur commented on a change in pull request #1820:
URL: https://github.com/apache/hadoop/pull/1820#discussion_r412295588
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/DynamicIOStatistics.java
##
@@ -0,0 +1,114 @@
+/*
+ * 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.hadoop.fs.statistics.impl;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.TreeMap;
+import java.util.function.ToLongFunction;
+
+import org.apache.hadoop.fs.statistics.IOStatistics;
+
+/**
+ * These statistics are dynamically evaluated by the supplied
+ * String - Long functions.
+ *
+ * This allows statistic sources to supply a list of callbacks used to
+ * generate the statistics on demand; similar to some of the Coda Hale metrics.
+ *
+ * The evaluation actually takes place during the iteration's {@code next()}
+ * call; the returned a value is fixed.
+ */
+final class DynamicIOStatistics implements IOStatistics {
+
+ /**
+ * Treemaps sort their insertions so the iterator is ordered.
+ */
+ private final Map> evaluators
Review comment:
What is the intent of ToLong function here? Why can't we use Long
directly?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] mukund-thakur commented on a change in pull request #1820: HADOOP-16830. Add public IOStatistics API + S3A implementation
mukund-thakur commented on a change in pull request #1820:
URL: https://github.com/apache/hadoop/pull/1820#discussion_r412295588
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/DynamicIOStatistics.java
##
@@ -0,0 +1,114 @@
+/*
+ * 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.hadoop.fs.statistics.impl;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.TreeMap;
+import java.util.function.ToLongFunction;
+
+import org.apache.hadoop.fs.statistics.IOStatistics;
+
+/**
+ * These statistics are dynamically evaluated by the supplied
+ * String - Long functions.
+ *
+ * This allows statistic sources to supply a list of callbacks used to
+ * generate the statistics on demand; similar to some of the Coda Hale metrics.
+ *
+ * The evaluation actually takes place during the iteration's {@code next()}
+ * call; the returned a value is fixed.
+ */
+final class DynamicIOStatistics implements IOStatistics {
+
+ /**
+ * Treemaps sort their insertions so the iterator is ordered.
+ */
+ private final Map> evaluators
Review comment:
What is the intent of ToLong function here? Why can't we use Long
directly?
This is the "Dynamic" stats source -you build it with functions and whenever
that stat is retrieved, it's re-evaluated. In implementation's I'm providing
getters to values
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] mukund-thakur commented on a change in pull request #1820: HADOOP-16830. Add public IOStatistics API + S3A implementation
mukund-thakur commented on a change in pull request #1820:
URL: https://github.com/apache/hadoop/pull/1820#discussion_r413912067
##
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInstrumentation.java
##
@@ -622,7 +642,8 @@ public void close() {
throttleRateQuantile.stop();
s3GuardThrottleRateQuantile.stop();
metricsSystem.unregisterSource(metricsSourceName);
- int activeSources = --metricsSourceActiveCounter;
+ metricsSourceActiveCounter--;
Review comment:
This was correct as well though a bit confusing. I think that is why you
changed it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] mukund-thakur commented on a change in pull request #1820: HADOOP-16830. Add public IOStatistics API + S3A implementation
mukund-thakur commented on a change in pull request #1820:
URL: https://github.com/apache/hadoop/pull/1820#discussion_r413027323
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/statistics/TestEmptyIOStatistics.java
##
@@ -0,0 +1,82 @@
+/*
+ * 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.hadoop.fs.statistics;
+
+import java.util.Iterator;
+import java.util.Map;
+
+import org.junit.Test;
+
+import org.apache.hadoop.fs.statistics.impl.EmptyIOStatistics;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertDoesNotHaveAttribute;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertHasAttribute;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertStatisticTracked;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertStatisticUnknown;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertStatisticUntracked;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertStatisticHasValue;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * Test handling of the empty IO statistics class.
+ */
+public class TestEmptyIOStatistics extends AbstractHadoopTestBase {
+
+ private final IOStatistics stats = EmptyIOStatistics.getInstance();
+
+ @Test
+ public void testAttributes() throws Throwable {
+assertHasAttribute(stats, IOStatistics.Attributes.Static);
+assertDoesNotHaveAttribute(stats, IOStatistics.Attributes.Dynamic);
+assertDoesNotHaveAttribute(stats, IOStatistics.Attributes.Snapshotted);
+ }
+
+ @Test
+ public void testSnapshotUnsupported() throws Throwable {
+assertThat(stats.snapshot())
+.describedAs("Snapshot of %s", stats)
+.isFalse();
+ }
+
+ @Test
+ public void testIterator() throws Throwable {
+Iterator> iterator = stats.iterator();
+
+assertThat(iterator.hasNext())
+.describedAs("iterator.hasNext()")
+.isFalse();
+assertThatThrownBy(iterator::next);
+ }
+
+ @Test
+ public void testUnknownStatistic() throws Throwable {
+assertStatisticUnknown(stats, "anything");
+assertStatisticUntracked(stats, "anything");
+
+// These actually test the assertion coverage
+assertThatThrownBy(() ->
Review comment:
Do you mind explaining a bit more for this. Thanks
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] mukund-thakur commented on a change in pull request #1820: HADOOP-16830. Add public IOStatistics API + S3A implementation
mukund-thakur commented on a change in pull request #1820:
URL: https://github.com/apache/hadoop/pull/1820#discussion_r413025331
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/statistics/TestEmptyIOStatistics.java
##
@@ -0,0 +1,82 @@
+/*
+ * 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.hadoop.fs.statistics;
+
+import java.util.Iterator;
+import java.util.Map;
+
+import org.junit.Test;
+
+import org.apache.hadoop.fs.statistics.impl.EmptyIOStatistics;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertDoesNotHaveAttribute;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertHasAttribute;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertStatisticTracked;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertStatisticUnknown;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertStatisticUntracked;
+import static
org.apache.hadoop.fs.statistics.IOStatisticAssertions.assertStatisticHasValue;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * Test handling of the empty IO statistics class.
+ */
+public class TestEmptyIOStatistics extends AbstractHadoopTestBase {
+
+ private final IOStatistics stats = EmptyIOStatistics.getInstance();
+
+ @Test
+ public void testAttributes() throws Throwable {
+assertHasAttribute(stats, IOStatistics.Attributes.Static);
+assertDoesNotHaveAttribute(stats, IOStatistics.Attributes.Dynamic);
+assertDoesNotHaveAttribute(stats, IOStatistics.Attributes.Snapshotted);
+ }
+
+ @Test
+ public void testSnapshotUnsupported() throws Throwable {
+assertThat(stats.snapshot())
+.describedAs("Snapshot of %s", stats)
+.isFalse();
+ }
+
+ @Test
+ public void testIterator() throws Throwable {
+Iterator> iterator = stats.iterator();
+
+assertThat(iterator.hasNext())
+.describedAs("iterator.hasNext()")
+.isFalse();
+assertThatThrownBy(iterator::next);
Review comment:
Should be asserting NoSuchElementException()
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] mukund-thakur commented on a change in pull request #1820: HADOOP-16830. Add public IOStatistics API + S3A implementation
mukund-thakur commented on a change in pull request #1820:
URL: https://github.com/apache/hadoop/pull/1820#discussion_r412750866
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/DynamicIOStatisticsBuilder.java
##
@@ -0,0 +1,112 @@
+/*
+ * 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.hadoop.fs.statistics.impl;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.ToLongFunction;
+
+import org.apache.hadoop.fs.statistics.IOStatistics;
+import org.apache.hadoop.metrics2.lib.MutableCounterLong;
+
+import static com.google.common.base.Preconditions.checkState;
+
+/**
+ * Builder of Dynamic IO Statistics.
+ * Instantiate through
+ * {@link IOStatisticsSupport#createDynamicIOStatistics()}.
+ */
+public class DynamicIOStatisticsBuilder {
+
+ /**
+ * the instance being built up. Will be null after the (single)
+ * call to {@link #build()}.
+ */
+ private DynamicIOStatistics instance = new DynamicIOStatistics();
+
+ /**
+ * Add a new evaluator to the statistics being built up.
+ * @param key key of this statistic
+ * @param eval evaluator for the statistic
+ * @return the builder.
+ */
+ public DynamicIOStatisticsBuilder add(String key,
+ ToLongFunction eval) {
+activeInstance().add(key, eval);
+return this;
+ }
+
+ /**
+ * Add a new evaluator to the statistics being built up.
+ * @param key key of this statistic
+ * @param source atomic counter
+ * @return the builder.
+ */
+ public DynamicIOStatisticsBuilder add(String key,
+ AtomicLong source) {
+add(key, s -> source.get());
+return this;
+ }
+
+ /**
+ * Add a new evaluator to the statistics being built up.
+ * @param key key of this statistic
+ * @param source atomic counter
Review comment:
nit: atomic integer counter. Since this is the only difference , I think
we should mention.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] mukund-thakur commented on a change in pull request #1820: HADOOP-16830. Add public IOStatistics API + S3A implementation
mukund-thakur commented on a change in pull request #1820:
URL: https://github.com/apache/hadoop/pull/1820#discussion_r412750424
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/DynamicIOStatisticsBuilder.java
##
@@ -0,0 +1,112 @@
+/*
+ * 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.hadoop.fs.statistics.impl;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.ToLongFunction;
+
+import org.apache.hadoop.fs.statistics.IOStatistics;
+import org.apache.hadoop.metrics2.lib.MutableCounterLong;
+
+import static com.google.common.base.Preconditions.checkState;
+
+/**
+ * Builder of Dynamic IO Statistics.
+ * Instantiate through
+ * {@link IOStatisticsSupport#createDynamicIOStatistics()}.
+ */
+public class DynamicIOStatisticsBuilder {
+
+ /**
+ * the instance being built up. Will be null after the (single)
+ * call to {@link #build()}.
+ */
+ private DynamicIOStatistics instance = new DynamicIOStatistics();
+
+ /**
+ * Add a new evaluator to the statistics being built up.
+ * @param key key of this statistic
+ * @param eval evaluator for the statistic
+ * @return the builder.
+ */
+ public DynamicIOStatisticsBuilder add(String key,
+ ToLongFunction eval) {
+activeInstance().add(key, eval);
+return this;
+ }
+
+ /**
+ * Add a new evaluator to the statistics being built up.
+ * @param key key of this statistic
+ * @param source atomic counter
Review comment:
nit: atomic long counter.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] mukund-thakur commented on a change in pull request #1820: HADOOP-16830. Add public IOStatistics API + S3A implementation
mukund-thakur commented on a change in pull request #1820:
URL: https://github.com/apache/hadoop/pull/1820#discussion_r412295588
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/DynamicIOStatistics.java
##
@@ -0,0 +1,114 @@
+/*
+ * 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.hadoop.fs.statistics.impl;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.TreeMap;
+import java.util.function.ToLongFunction;
+
+import org.apache.hadoop.fs.statistics.IOStatistics;
+
+/**
+ * These statistics are dynamically evaluated by the supplied
+ * String - Long functions.
+ *
+ * This allows statistic sources to supply a list of callbacks used to
+ * generate the statistics on demand; similar to some of the Coda Hale metrics.
+ *
+ * The evaluation actually takes place during the iteration's {@code next()}
+ * call; the returned a value is fixed.
+ */
+final class DynamicIOStatistics implements IOStatistics {
+
+ /**
+ * Treemaps sort their insertions so the iterator is ordered.
+ */
+ private final Map> evaluators
Review comment:
What is the intent of ToLong function here? Why can't we use Long
directly?
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/IOStatistics.java
##
@@ -0,0 +1,142 @@
+/*
+ * 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.hadoop.fs.statistics;
+
+import java.util.Map;
+
+import org.apache.hadoop.classification.InterfaceStability;
+
+/**
+ * IO Statistics.
+ *
+ * These are low-cost per-instance statistics provided by any IO components.
+ *
+ * The statistics MUST BE for the specific instance of the source;
+ * possibly including aggregate statistics from other objects
+ * created by that stores.
+ * For example, the statistics of a filesystem instance must be unique
+ * to that instant and not shared with any other.
+ * However, those statistics may also collect and aggregate statistics
+ * generated in the use of input and output streams created by that
+ * file system instance.
+ *
+ *
+ * The iterator is a possibly empty iterator over all monitored statistics.
+ *
+ *
+ * The attributes of an instance can be probed for with
+ * {@link #hasAttribute(Attributes)}.
+ *
+ *
+ * The set of statistic keys SHOULD remain unchanged, and MUST NOT
+ * ever remove keys.
+ *
+ * The statistics MAY BE dynamic: every call to {@code iterator()}
+ * MAY return a current/recent set of statistics.
+ * This
+ *
+ *
+ * The values MAY change across invocations of {@code iterator()}.
+ *
+ *
+ * The update MAY be in the iterable() call, or MAY be in the actual
+ * Iterable.next() operation.
+ *
+ *
+ * The returned Map.Entry instances MUST return the same value on
+ * repeated getValue() calls.
+ *
+ *
+ * Queries of statistics SHOULD Be fast and Nonblocking to the extent
+ * that if invoked during a long operation, they will prioritize
+ * returning fast over most timely values.
+ *
+ *
+ * The statistics MAY lag; especially for statistics collected in separate
+ * operations (e.g stream IO statistics as provided by a filesystem
+ * instance).
+ *
+ *
+ * Thread safety: an instance of IOStatistics can be shared across threads;
+ * a call to @code iterator()} is
