Skip to content

Conversation

@SaketaChalamchala
Copy link
Contributor

@SaketaChalamchala SaketaChalamchala commented Aug 15, 2023

What changes were proposed in this pull request?

Current implementation returns a null and later on ScmBlockLocationProtocolServerSideTranslatorPB#allocateScmBlock() throws an exception with #blocks allocated and requested.

# hdfs dfs -put /etc/krb5.conf ofs://ozone1/vol1/bucket/3
put: Allocated 0 blocks. Requested 1 blocks

Proposed changed returns an exception when a block cannot be allocated after trying all possible strategies

What is the link to the Apache JIRA

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

How was this patch tested?

Manual.

sh-4.2$ ozone sh key put /vol1/bucket1/key1 CONTRIBUTING.md
INTERNAL_ERROR Unable to allocate a container to the block of size: 1048576, replicationConfig: RATIS/THREE. Unable to find enough nodes that meet the space requirement of 10485760 bytes for metadata and 1073741824 bytes for data in healthy node set. Required 3. Found 2.
sh-4.2$ 

@errose28
Copy link
Contributor

Thanks for improving this @SaketaChalamchala, this message has been confusing for users before. Could we also add a message summarizing why the allocation failed? For example, the log lines farther up in getContainer provide more information to the SCM log, but those messages could be returned to the client as well.

@SaketaChalamchala
Copy link
Contributor Author

Thanks for improving this @SaketaChalamchala, this message has been confusing for users before. Could we also add a message summarizing why the allocation failed? For example, the log lines farther up in getContainer provide more information to the SCM log, but those messages could be returned to the client as well.

Done.

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

Functionally LGTM just minor comments remaining.

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @SaketaChalamchala. CI will likely fail until #5217 is merged (should be soon). I will hold off running CI on this PR until that is resolved.

@errose28
Copy link
Contributor

errose28 commented Sep 5, 2023

Looks like #5217 is not progressing as fast as I thought, I will go ahead with the CI on this PR.

@errose28
Copy link
Contributor

errose28 commented Sep 6, 2023

CI failures are fixed by #5217

@errose28 errose28 merged commit c5998ce into apache:master Sep 6, 2023
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Feb 1, 2024
…located with RATIS THREE. (apache#5189)

(cherry picked from commit c5998ce)
Change-Id: Ica0896fc6a7f0f2d0760948c14b5b284ff7f9df9
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.

2 participants