You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2023/01/09 08:31:08 UTC
[GitHub] [ozone] ArafatKhan2198 opened a new pull request, #4158: HDDS-6056 Recon /containers endpoint should return SCM container data instead of OM container data.
ArafatKhan2198 opened a new pull request, #4158:
URL: https://github.com/apache/ozone/pull/4158
## What changes were proposed in this pull request?
**Problem :**
- The problem is that currently we are iterating through the OM-DB Snapshot using the ReconContainerMetadataManagerto provide data to these endpoints. Which is not very Reliable and UpToDate for container metadata. On the other hand we know for a fact that SCM DB snapshots are also fetched by recon which have a more reliable and upToDate information about the containters. Hence, recon should use its SCM metadata to give out information through the Container Endpoint.
**Solution:**
- Its sugested that SCM Snapshots have a better upTodate information about the containers, To get the required data from these snapshots, we will fetch data from `ReconContainerManager` Class which interacts with the SCM snapshots and use its methods to populate the results of `ContainerEndpoint` .
## What is the link to the Apache JIRA
(Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)
Please replace this section with the link to the Apache JIRA)
## How was this patch tested?
(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1422682294
Thanks @ArafatKhan2198 for the fix, @devmadhuu, @dombizita, @kerneltime for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a diff in pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #4158:
URL: https://github.com/apache/ozone/pull/4158#discussion_r1083653560
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -112,21 +117,29 @@ public Response getContainers(
int limit,
@DefaultValue(PREV_CONTAINER_ID_DEFAULT_VALUE)
@QueryParam(RECON_QUERY_PREVKEY) long prevKey) {
- Map<Long, ContainerMetadata> containersMap;
- long containersCount;
- try {
- containersMap =
- reconContainerMetadataManager.getContainers(limit, prevKey);
- containersCount = reconContainerMetadataManager.getCountForContainers();
- } catch (IOException ioEx) {
- throw new WebApplicationException(ioEx,
- Response.Status.INTERNAL_SERVER_ERROR);
+ if (limit < 0 || prevKey < 0) {
+ // Send back an empty response
+ return Response.ok().build();
Review Comment:
Should the return here be something other than `ok` maybe `notAcceptable`?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1422226389
@ArafatKhan2198 please go ahead and remove the obsolete integration test to allow successful CI in this PR.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dombizita commented on a diff in pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4158:
URL: https://github.com/apache/ozone/pull/4158#discussion_r1085133402
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/scm/TestReconContainerManager.java:
##########
@@ -47,8 +47,10 @@
import org.apache.hadoop.hdds.scm.container.common.helpers.ContainerWithPipeline;
import org.apache.hadoop.hdds.scm.ha.SCMHAManager;
import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+
import org.junit.jupiter.api.Test;
+
Review Comment:
please remove added empty lines
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1400008707
@ArafatKhan2198 - changes looks good.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1418194803
@ArafatKhan2198 Please check failure in `TestReconWithOzoneManager`, it seems to be related.
```
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 34.526 s <<< FAILURE! - in org.apache.hadoop.ozone.recon.TestReconWithOzoneManager
org.apache.hadoop.ozone.recon.TestReconWithOzoneManager.testOmDBSyncing Time elapsed: 0.859 s <<< FAILURE!
java.lang.AssertionError: expected:<1> but was:<0>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:647)
at org.junit.Assert.assertEquals(Assert.java:633)
at org.apache.hadoop.ozone.recon.TestReconWithOzoneManager.testOmDBSyncing(TestReconWithOzoneManager.java:212)
```
https://github.com/apache/ozone/actions/runs/3984351091/jobs/6830477229#step:5:3162
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1418999212
> > @ArafatKhan2198 Please check failure in `TestReconWithOzoneManager`, it seems to be related.
> > ```
> > Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 34.526 s <<< FAILURE! - in org.apache.hadoop.ozone.recon.TestReconWithOzoneManager
> > org.apache.hadoop.ozone.recon.TestReconWithOzoneManager.testOmDBSyncing Time elapsed: 0.859 s <<< FAILURE!
> > java.lang.AssertionError: expected:<1> but was:<0>
> > at org.junit.Assert.fail(Assert.java:89)
> > at org.junit.Assert.failNotEquals(Assert.java:835)
> > at org.junit.Assert.assertEquals(Assert.java:647)
> > at org.junit.Assert.assertEquals(Assert.java:633)
> > at org.apache.hadoop.ozone.recon.TestReconWithOzoneManager.testOmDBSyncing(TestReconWithOzoneManager.java:212)
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > https://github.com/apache/ozone/actions/runs/3984351091/jobs/6830477229#step:5:3162
>
> Hi @adoroszlai The UT test testOmDBSyncing() in `TestReconWithOzoneManager` is testing whether the container data fetched from the `containerEndpoint` is in sync with its data source, which is the metadata from the **OM-DB Snapshot** that is maintained within recon. However, as per the recent JIRA, we have decided to switch from **OM-DB Snapshot** to **SCM DB Snapshots** for more reliable and up-to-date information about the containers. I have already written UT's in TestContainerEndpoint to test these changes. Thus, this Unit Test seems irrelevant as we are no longer using OM-DB for information. Should we discard this UT or write a similar one for SCM?
@ArafatKhan2198 , Please verify if you can change the assertion source as per your container information from SCM, so that test case is passed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1418899863
@ArafatKhan2198 I cannot answer that question, let's ask folks more familiar with Recon. @devmadhuu @dombizita @smengcl what do you think?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #4158:
URL: https://github.com/apache/ozone/pull/4158
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ArafatKhan2198 commented on pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1422215432
@devmadhuu @adoroszlai
The `TestReconWithOzoneManager` test class verifies information from the OzoneManager and is designed specifically for this purpose. Adding SCM as a new source in this class would not align with its original intent. Making significant changes to the code for testing a single endpoint undermines the purpose of the class. Fortunately, the functionality we aim to test is already covered by `TestContainerEndpoint`.
Given that there is currently no class for handling similar integration testing for SCM recon, I suggest creating a new JIRA to do integration tests on all endpoints that utilize SCM data. This work falls outside the scope of the present JIRA and is better suited for separate consideration. I have already tested my code changes in TestContainerEndpoint, which should meet our needs for the time being.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4158:
URL: https://github.com/apache/ozone/pull/4158#discussion_r1096720465
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/scm/TestReconContainerManager.java:
##########
@@ -47,8 +47,10 @@
import org.apache.hadoop.hdds.scm.container.common.helpers.ContainerWithPipeline;
import org.apache.hadoop.hdds.scm.ha.SCMHAManager;
import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+
import org.junit.jupiter.api.Test;
+
Review Comment:
```suggestion
```
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/scm/TestReconContainerManager.java:
##########
@@ -47,8 +47,10 @@
import org.apache.hadoop.hdds.scm.container.common.helpers.ContainerWithPipeline;
import org.apache.hadoop.hdds.scm.ha.SCMHAManager;
import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ArafatKhan2198 commented on pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1418638474
> @ArafatKhan2198 Please check failure in `TestReconWithOzoneManager`, it seems to be related.
>
> ```
> Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 34.526 s <<< FAILURE! - in org.apache.hadoop.ozone.recon.TestReconWithOzoneManager
> org.apache.hadoop.ozone.recon.TestReconWithOzoneManager.testOmDBSyncing Time elapsed: 0.859 s <<< FAILURE!
> java.lang.AssertionError: expected:<1> but was:<0>
> at org.junit.Assert.fail(Assert.java:89)
> at org.junit.Assert.failNotEquals(Assert.java:835)
> at org.junit.Assert.assertEquals(Assert.java:647)
> at org.junit.Assert.assertEquals(Assert.java:633)
> at org.apache.hadoop.ozone.recon.TestReconWithOzoneManager.testOmDBSyncing(TestReconWithOzoneManager.java:212)
> ```
>
> https://github.com/apache/ozone/actions/runs/3984351091/jobs/6830477229#step:5:3162
Hi @adoroszlai
The UT test testOmDBSyncing() in `TestReconWithOzoneManager` is testing whether the container data fetched from the `containerEndpoint` is in sync with its data source, which is the metadata from the **OM-DB Snapshot** that is maintained within recon. However, as per the recent JIRA, we have decided to switch from **OM-DB Snapshot** to **SCM DB Snapshots** for more reliable and up-to-date information about the containers.
I have already written UT's in TestContainerEndpoint to test these changes. Thus, this Unit Test seems irrelevant as we are no longer using OM-DB for information. Should we discard this UT or write a similar one for SCM?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ArafatKhan2198 commented on pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1422568282
@adoroszlai I believe we can merge it now !!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #4158: HDDS-6056 Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #4158:
URL: https://github.com/apache/ozone/pull/4158#issuecomment-1375961928
@dombizita @devmadhuu can you'll please take a look?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ArafatKhan2198 commented on a diff in pull request #4158: HDDS-6056. Recon /containers endpoint should return SCM container data instead of OM container data.
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #4158:
URL: https://github.com/apache/ozone/pull/4158#discussion_r1083729521
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -112,21 +117,29 @@ public Response getContainers(
int limit,
@DefaultValue(PREV_CONTAINER_ID_DEFAULT_VALUE)
@QueryParam(RECON_QUERY_PREVKEY) long prevKey) {
- Map<Long, ContainerMetadata> containersMap;
- long containersCount;
- try {
- containersMap =
- reconContainerMetadataManager.getContainers(limit, prevKey);
- containersCount = reconContainerMetadataManager.getCountForContainers();
- } catch (IOException ioEx) {
- throw new WebApplicationException(ioEx,
- Response.Status.INTERNAL_SERVER_ERROR);
+ if (limit < 0 || prevKey < 0) {
+ // Send back an empty response
+ return Response.ok().build();
Review Comment:
Yes, that is a good suggestion!
It would be more appropriate to return a "not acceptable" status code (HTTP status code 406) if the limit or prevKey is negative, as that shows that the request is not valid.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org