fuatbasik commented on code in PR #7763:
URL: https://github.com/apache/hadoop/pull/7763#discussion_r2195042590
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Statistic.java:
##########
@@ -459,6 +459,14 @@ public enum Statistic {
"Gauge of active memory in use",
TYPE_GAUGE),
+ ANALYTICS_GET_REQUESTS(
+ StreamStatisticNames.STREAM_READ_ANALYTICS_GET_REQUESTS,
+ "GET requests made by analytics streams",
Review Comment:
This is nit but i believe an important one. We have to make sure we are
giving right name, description as well as doing it in the right place.
1/ why are these are here at these lines? This is neither lexicographic nor
following a logical separation. We probably need to move these up to Object IO
section.
2/ Next is the name for Stream read we had `STREAM_READ_OPENED` and we added
`STREAM_READ_ANALYTICS_OPENED` so following the same schema we probably need to
re-name this to `OBJECT_GET_ANALYTICS_REQUESTS` (there is no GET equivalent
currently because stream read opens = gets. Likewise for HEAD, we have
`OBJECT_METADATA_REQUESTS`, which we can add
`OBJECT_METADATA_ANALYTICS_REQUESTS`.
3/ we should make sure the messages are similar to others. For example, we
should call this `Count of object get requests made by analytics stream`. and
for head `Count of requests for object metadata made by analytics stream`
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/AnalyticsStream.java:
##########
@@ -63,13 +68,23 @@ public AnalyticsStream(final ObjectReadParameters
parameters,
@Override
public int read() throws IOException {
throwIfClosed();
+ if (getPos() >= lengthLimit) {
+ return -1; // EOF reached due to length limit
Review Comment:
do we need to close underlying stream here before returning? We might check
this from other Stream implementations.
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/AnalyticsRequestCallback.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.s3a.impl.streams;
+
+import org.apache.hadoop.fs.s3a.statistics.S3AInputStreamStatistics;
+import org.apache.hadoop.fs.statistics.DurationTracker;
+import software.amazon.s3.analyticsaccelerator.util.RequestCallback;
+
+/**
+ * Implementation of AAL's RequestCallback interface that tracks analytics
operations.
+ */
+public class AnalyticsRequestCallback implements RequestCallback {
+ private final S3AInputStreamStatistics statistics;
+
+ /**
+ * Create a new callback instance.
+ * @param statistics the statistics to update
+ */
+ public AnalyticsRequestCallback(S3AInputStreamStatistics statistics) {
+ this.statistics = statistics;
+ }
+
+ @Override
+ public void onGetRequest() {
+ statistics.incrementAnalyticsGetRequests();
+ // Update ACTION_HTTP_GET_REQUEST statistic
+ DurationTracker tracker = statistics.initiateGetRequest();
Review Comment:
this duration will be always very close to 0 right? Why do we need to start
and close tracker?
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/commit/ITestCommitOperationCost.java:
##########
@@ -174,10 +173,7 @@ private void abortActiveStream() throws IOException {
public void testCostOfCreatingMagicFile() throws Throwable {
describe("Files created under magic paths skip existence checks and marker
deletes");
- // Analytics accelerator currently does not support IOStatistics, this
will be added as
- // part of https://issues.apache.org/jira/browse/HADOOP-19364
- skipIfAnalyticsAcceleratorEnabled(getConfiguration(),
- "Analytics Accelerator currently does not support stream statistics");
+
Review Comment:
nit: extra whitespace
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/AnalyticsRequestCallback.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.s3a.impl.streams;
+
+import org.apache.hadoop.fs.s3a.statistics.S3AInputStreamStatistics;
+import org.apache.hadoop.fs.statistics.DurationTracker;
+import software.amazon.s3.analyticsaccelerator.util.RequestCallback;
+
+/**
+ * Implementation of AAL's RequestCallback interface that tracks analytics
operations.
+ */
+public class AnalyticsRequestCallback implements RequestCallback {
+ private final S3AInputStreamStatistics statistics;
+
+ /**
+ * Create a new callback instance.
+ * @param statistics the statistics to update
+ */
+ public AnalyticsRequestCallback(S3AInputStreamStatistics statistics) {
+ this.statistics = statistics;
+ }
+
+ @Override
+ public void onGetRequest() {
+ statistics.incrementAnalyticsGetRequests();
+ // Update ACTION_HTTP_GET_REQUEST statistic
+ DurationTracker tracker = statistics.initiateGetRequest();
+ tracker.close();
+ }
+
+ @Override
+ public void onHeadRequest() {
+ statistics.incrementAnalyticsHeadRequests();
Review Comment:
now on GET you are updating both Analytics request counter and
ACTION_HTTP_GET_REQUEST (with tracker). Why are you not updating
OBJECT_METADATA_REQUESTS here?
--
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.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
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
