Skip to content

Conversation

@umamaheswararao
Copy link
Contributor

What changes were proposed in this pull request?

Updated the doc for EC

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6172

How was this patch tested?

NA

@umamaheswararao
Copy link
Contributor Author

Updated the initial version of the doc. Please review!

Below diagram depicts the block allocation in containers as logical groups.
For interest of space, we assumed EC(3, 2) replication config for the diagram.

![EC Block Allocation in Containers](EC-Write-Block-Allocation-in-Containers.png)
Copy link
Contributor

@JyotinderSingh JyotinderSingh Jan 21, 2022

Choose a reason for hiding this comment

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

Could you change the Markdown Image tag to the Hugo shortcode defined in hadoop-hdds/docs/themes/ozonedoc/layouts/shortcodes/image.html
This would ensure that the images don't overflow content boundaries on the website.

You can change it to the following:

{{< image src="EC-Write-Block-Allocation-in-Containers.png">}}

Once you do this, the image won't be visible on the intellij markdown editor, but will be available on the website after the Hugo build process.
You can preview the website as it will appear on the web by running the following (reference):

hugo serve

note: you will need to have hugo available on your machine to run the serve command brew install hugo


Let's zoom out the blockID: 1 data layout from the above picture, that showed in the following picture.
This picture shows how the chunks will be layed out in data node blocks.
![EC Chunk Layout](EC-Chunk-Layout.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to use the Hugo shortcode:

{{< image src="EC-Chunk-Layout.png">}}

it will attempt to do plain reads chunk by chunk in round robin fashion from d data blocks.

Below picture shows the order when there are no failures while reading.
![EC Reads With no Failures](EC-Reads-With-No-Failures.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to use the Hugo shortcode:

{{< image src="EC-Reads-With-No-Failures.png">}}

This reconstruction is completely transparent to the applications.

Below picture depicts how it uses parity replicas in reconstruction.
![EC Reconstructional Reads](EC-Reconstructional-Read.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to use the Hugo shortcode:

{{< image src="EC-Reconstructional-Read.png">}}

Copy link
Contributor Author

@umamaheswararao umamaheswararao Jan 21, 2022

Choose a reason for hiding this comment

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

Looks like many of the ozone docs followed the above approach. (ex: OM-HA.md file image refs used ![Double buffer](HA-OM-doublebuffer.png)) I already ran mvn site and it's generated fine to me.
Is there an issue with existing way to referencing? ( This is working for both intellij and site to me)
Please check this mvn site generated file screenshot

Copy link
Contributor

@JyotinderSingh JyotinderSingh Jan 21, 2022

Choose a reason for hiding this comment

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

The existing approach works fine for larger screens but can cause the image to overflow out of the screen when viewing the site on smaller screens since it does not scale down the image automatically. You can try to reproduce the behavior by changing the viewport size by activating developer tools in chrome.

Like in the image below, the image goes out to the right while the text is pushed to the left.

Screenshot 2022-01-21 at 5 10 33 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @JyotinderSingh , Thank you for providing details.
Not sure users will really view these docs in smaller screens. Dropping support to view in github and intellij is concerning me where we could allow reviewer view the images easily and check whether they embed into docs properly( without needing to run site).
I kept all images size 5 inch range size.

If no one else has any concern, then I will go update it. Thanks for checking the docs and providing your comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @umamaheswararao is right here, our doc uses this notation where I quickly checked, so I would suggest to stick to this notation for now.

If we want to solve the small screen problem, let's do it in a separate JIRA, I would do it differently though, as it is way more comfortable to review these files in IDE, and over Github if images are shown, so we may change the notation during site build, and than use hugo to work on the md files modified during the first step. With that we can have the advantages of both notations where we need it.

@JyotinderSingh what do you think? Would that be a feasible way to solve this problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this reasoning @umamaheswararao @fapifta.
Ease of reviewing is definitely a benefit and @fapifta's suggestion of changing it during build time looks like a good solution to this problem.
I'll take this forward as another jira.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @fapifta for chime in and providing your views.
@JyotinderSingh, very cool. Let's file separate JIRA and investigate how we could replace while building hugo. Thanks for offering the help here.

@JyotinderSingh
Copy link
Contributor

Thank you for the docs @umamaheswararao! I have added a few minor formatting-related comments.

@fapifta
Copy link
Contributor

fapifta commented Jan 25, 2022

Hi @umamaheswararao,

thank you for writing the documentation parts for the EC feature. I have added a couple of inline comments mainly for spelling issues, or where I have not understood well the sentence for the first read. (It might be because of my non-native english skills, so I might not be right everywhere).

In general I would like to ask you to proof read the text one more time and please take care of some inconsistencies in writing different names. What I found inconsistent is the mixing of lower/uppercase forms like:
ec vs Ec vs EC
erasure coding vs Erasure coding vs Erasure Coding
replication config vs Replication config vs Replication configuration mixed with an ec prefix sometimes

At some points while I was reading I really missed an article in front of some words, and sometimes I felt the one I see is not really necessary. Again this can be my non-nativeness, and you might be perfectly right with the usage or lack of the article, hence when you read the text again, please consider this, and if articles are really missed or if they are not needed then please fix it.

Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

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

Hi @umamaheswararao thank you for addressing my concerns. I am ok with the current version as the first doc version, later on we might extend and make it more precise as needed anyways. +1 from my side.

@umamaheswararao
Copy link
Contributor Author

umamaheswararao commented Jan 25, 2022

@fapifta I totally agree that there are lot of places I used different formats as you mentioned. I did not really focused on that EC and Erasure Coding or EC Replication Config or EC replication configuration(I meant to say it's erasure coding configuration, but indirectly it's bringing inconsistencies for readers probably) is creating some inconsistencies. As a review reader, I feel your views are correct than my biased view :-)
I changed most of the places to be in consistent now. I still used Replication Config in place to tell that an interface.

Thanks for the reviews. Yeah we can continue improve as we need few mores updates to this docs as we may need to update few other configs.

@fapifta
Copy link
Contributor

fapifta commented Jan 25, 2022

Thank you @umamaheswararao +1 on committing it, after we discussed with @JyotinderSingh the hugo vs simple markup of images as well.

@umamaheswararao umamaheswararao merged commit b40ab3f into apache:HDDS-3816-ec Jan 26, 2022
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.

4 participants