Skip to content

Conversation

@timmylicheng
Copy link
Contributor

What changes were proposed in this pull request?

Add failover proxy for SCM container client
(Please fill in changes proposed in this fix)

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4191
(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?

UT
(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)

@codecov-io
Copy link

Codecov Report

Merging #1514 into HDDS-2823 will decrease coverage by 0.02%.
The diff coverage is 74.61%.

Impacted file tree graph

@@               Coverage Diff               @@
##             HDDS-2823    #1514      +/-   ##
===============================================
- Coverage        73.76%   73.74%   -0.03%     
- Complexity       10466    10489      +23     
===============================================
  Files             1002     1003       +1     
  Lines            51756    51857     +101     
  Branches          5008     5019      +11     
===============================================
+ Hits             38180    38241      +61     
- Misses           11170    11205      +35     
- Partials          2406     2411       +5     
Impacted Files Coverage Δ Complexity Δ
...ntainerLocationProtocolServerSideTranslatorPB.java 53.16% <14.28%> (-1.19%) 26.00 <2.00> (+1.00) ⬇️
...oxy/SCMContainerLocationFailoverProxyProvider.java 76.85% <76.85%> (ø) 22.00 <22.00> (?)
...ntainerLocationProtocolClientSideTranslatorPB.java 54.70% <80.00%> (-0.39%) 28.00 <2.00> (ø)
...m/proxy/SCMBlockLocationFailoverProxyProvider.java 75.45% <100.00%> (ø) 22.00 <0.00> (ø)
.../hadoop/hdds/scm/cli/ContainerOperationClient.java 36.97% <100.00%> (-4.89%) 14.00 <1.00> (ø)
.../java/org/apache/hadoop/ozone/om/OzoneManager.java 68.12% <100.00%> (-0.17%) 207.00 <0.00> (ø)
...ache/hadoop/ozone/recon/ReconControllerModule.java 81.33% <100.00%> (+1.84%) 6.00 <0.00> (ø)
...ache/hadoop/ozone/om/codec/S3SecretValueCodec.java 90.90% <0.00%> (-9.10%) 3.00% <0.00%> (-1.00%)
...va/org/apache/hadoop/hdds/utils/db/RDBMetrics.java 92.85% <0.00%> (-7.15%) 14.00% <0.00%> (-1.00%)
...ozone/container/ozoneimpl/ContainerController.java 76.31% <0.00%> (-5.27%) 13.00% <0.00%> (-1.00%)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ddaa07...31ce4f6. Read the comment docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move line 82 into loadConfigs()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to keep it as final variable so that value is only set in constructor. What do you think?

@elek
Copy link
Member

elek commented Nov 12, 2020

Is this change compatible with the hadoop2.x client?

With OM we created OMTransport with two implementation:

  • one for hadoop3 which used the failover proxy
  • one for hadoop2 which is without failover proxy

I don't have the full context, but we may need a similar method here, too.

@timmylicheng
Copy link
Contributor Author

Is this change compatible with the hadoop2.x client?

With OM we created OMTransport with two implementation:

  • one for hadoop3 which used the failover proxy
  • one for hadoop2 which is without failover proxy

I don't have the full context, but we may need a similar method here, too.

@elek Hey Marton. I think the failover proxy here is a little different from OM's. Here we are retrying with IP addresses specified in SCM HA configurations to see who should be the current leader SCM to handle requests. It doesn't seem to involve hadoop version.

@elek
Copy link
Member

elek commented Nov 23, 2020

Thanks for the answer @timmylicheng

As far as I remember the FailoverProxyProvider caused the problem which is refactored between hadoop2 and hadoop3.

But thinking it again it shouldn't be a problem at all. I assume the client will use only OM calls as before, and we can use any hadoop3.x classes on server side. So my question doesn't have any sense ;-)

@ChenSammi
Copy link
Contributor

LGTM + 1.

@ChenSammi ChenSammi merged commit 285d793 into apache:HDDS-2823 Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants