You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "mladjan-gadzic (via GitHub)" <gi...@apache.org> on 2023/03/21 17:33:29 UTC
[GitHub] [ozone] mladjan-gadzic opened a new pull request, #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
mladjan-gadzic opened a new pull request, #4443:
URL: https://github.com/apache/ozone/pull/4443
## What changes were proposed in this pull request?
Add state field for container replica response for Recon API.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7098
## How was this patch tested?
- manual
- unit
--
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] errose28 commented on a diff in pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #4443:
URL: https://github.com/apache/ozone/pull/4443#discussion_r1197054018
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java:
##########
@@ -423,14 +430,15 @@ public void flushReplicaHistoryMapToDB(boolean clearMap) {
}
public void upsertContainerHistory(long containerID, UUID uuid, long time,
- long bcsId) {
+ long bcsId, String state) {
Map<UUID, ContainerReplicaHistory> tsMap;
try {
tsMap = cdbServiceProvider.getContainerReplicaHistory(containerID);
ContainerReplicaHistory ts = tsMap.get(uuid);
if (ts == null) {
// New entry
- tsMap.put(uuid, new ContainerReplicaHistory(uuid, time, time, bcsId));
+ tsMap.put(uuid, new ContainerReplicaHistory(uuid, time, time, bcsId,
+ state));
} else {
// Entry exists, update last seen time and put it back to DB.
ts.setLastSeenTime(time);
Review Comment:
Do we need to update the replica state here too? (in the else block)
--
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] errose28 commented on a diff in pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #4443:
URL: https://github.com/apache/ozone/pull/4443#discussion_r1197054018
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java:
##########
@@ -423,14 +430,15 @@ public void flushReplicaHistoryMapToDB(boolean clearMap) {
}
public void upsertContainerHistory(long containerID, UUID uuid, long time,
- long bcsId) {
+ long bcsId, String state) {
Map<UUID, ContainerReplicaHistory> tsMap;
try {
tsMap = cdbServiceProvider.getContainerReplicaHistory(containerID);
ContainerReplicaHistory ts = tsMap.get(uuid);
if (ts == null) {
// New entry
- tsMap.put(uuid, new ContainerReplicaHistory(uuid, time, time, bcsId));
+ tsMap.put(uuid, new ContainerReplicaHistory(uuid, time, time, bcsId,
+ state));
} else {
// Entry exists, update last seen time and put it back to DB.
ts.setLastSeenTime(time);
Review Comment:
Do we need to update the replica state here too?
--
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 #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4443:
URL: https://github.com/apache/ozone/pull/4443#issuecomment-1495704955
> > It would be ideal to reuse the State Enum type. Would the answer in this stackoverflow article help address the import problem? https://stackoverflow.com/questions/66524793/how-to-use-the-enum-of-one-proto-file-in-another-file
>
> I've tried it, but there is an issue with backward compatibility
Does the compatibility check also fail if the field is `optional` instead of `required`?
--
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] jojochuang commented on a diff in pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #4443:
URL: https://github.com/apache/ozone/pull/4443#discussion_r1149624235
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -405,6 +405,7 @@ message ContainerReplicaHistoryProto {
required int64 firstSeenTime = 2;
required int64 lastSeenTime = 3;
required int64 bcsId = 4;
+ required string state = 5;
Review Comment:
will it be backward compatible if it's a required field, or is backward compat not a concern 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: 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] jojochuang commented on a diff in pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #4443:
URL: https://github.com/apache/ozone/pull/4443#discussion_r1156466572
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -405,6 +405,7 @@ message ContainerReplicaHistoryProto {
required int64 firstSeenTime = 2;
required int64 lastSeenTime = 3;
required int64 bcsId = 4;
+ required string state = 5;
Review Comment:
It would break rolling upgrade, or when recon and SCM are in different versions.
I see that rolling upgrade is not yet a thing, so I am okay with it.
--
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 #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #4443:
URL: https://github.com/apache/ozone/pull/4443#discussion_r1175528420
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -405,6 +405,7 @@ message ContainerReplicaHistoryProto {
required int64 firstSeenTime = 2;
required int64 lastSeenTime = 3;
required int64 bcsId = 4;
+ required string state = 5;
Review Comment:
There needs to be a powerful reason for making the proto message `required`. I would change this to optional. If the code depends on it, it should handle it rather than having proto layer throw an exception on parsing.
--
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] mladjan-gadzic commented on pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on PR #4443:
URL: https://github.com/apache/ozone/pull/4443#issuecomment-1556783888
@errose28 thanks 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] mladjan-gadzic commented on a diff in pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on code in PR #4443:
URL: https://github.com/apache/ozone/pull/4443#discussion_r1197742342
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java:
##########
@@ -423,14 +430,15 @@ public void flushReplicaHistoryMapToDB(boolean clearMap) {
}
public void upsertContainerHistory(long containerID, UUID uuid, long time,
- long bcsId) {
+ long bcsId, String state) {
Map<UUID, ContainerReplicaHistory> tsMap;
try {
tsMap = cdbServiceProvider.getContainerReplicaHistory(containerID);
ContainerReplicaHistory ts = tsMap.get(uuid);
if (ts == null) {
// New entry
- tsMap.put(uuid, new ContainerReplicaHistory(uuid, time, time, bcsId));
+ tsMap.put(uuid, new ContainerReplicaHistory(uuid, time, time, bcsId,
+ state));
} else {
// Entry exists, update last seen time and put it back to DB.
ts.setLastSeenTime(time);
Review Comment:
Thanks for the review! Yes, you are right, I'll add that.
--
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] mladjan-gadzic commented on a diff in pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on code in PR #4443:
URL: https://github.com/apache/ozone/pull/4443#discussion_r1143780279
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -405,6 +405,7 @@ message ContainerReplicaHistoryProto {
required int64 firstSeenTime = 2;
required int64 lastSeenTime = 3;
required int64 bcsId = 4;
+ required string state = 5;
Review Comment:
An idea was to pull an enum from https://github.com/mladjan-gadzic/ozone/blob/HDDS-7098/hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto#L213 and use it here in order not to maintain same enum in two places but I could not make it. Because of that I decided not to use enum at all and stick with string instead.
--
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] mladjan-gadzic commented on pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on PR #4443:
URL: https://github.com/apache/ozone/pull/4443#issuecomment-1495699588
> It would be ideal to reuse the State Enum type. Would the answer in this stackoverflow article help address the import problem? https://stackoverflow.com/questions/66524793/how-to-use-the-enum-of-one-proto-file-in-another-file
I like that idea as well. I've tried it, but there is an issue with backward compatibility: `[ERROR] Failed to execute goal com.salesforce.servicelibs:proto-backwards-compatibility:1.0.7:backwards-compatibility-check (default) on project hdds-interface-server: Backwards compatibility check failed! You can override this by specifying allowBreakingChanges=true -> [Help 1]`. I'd say we should not break it, especially because I am not aware of the consequences it could have on the system
--
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 #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4443:
URL: https://github.com/apache/ozone/pull/4443#issuecomment-1556809910
Thanks @mladjan-gadzic for the patch, @devmadhuu, @errose28, @jojochuang, @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] mladjan-gadzic commented on a diff in pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on code in PR #4443:
URL: https://github.com/apache/ozone/pull/4443#discussion_r1150266523
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -405,6 +405,7 @@ message ContainerReplicaHistoryProto {
required int64 firstSeenTime = 2;
required int64 lastSeenTime = 3;
required int64 bcsId = 4;
+ required string state = 5;
Review Comment:
I'd say backward compatibility is not a concern 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: 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 #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4443:
URL: https://github.com/apache/ozone/pull/4443#issuecomment-1556806613
Failure of `TestOzoneManagerHASnapshot` is unrelated and it has been fixed on `master` since then.
--
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 #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #4443:
URL: https://github.com/apache/ozone/pull/4443
--
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] mladjan-gadzic commented on pull request #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on PR #4443:
URL: https://github.com/apache/ozone/pull/4443#issuecomment-1495711712
> > > It would be ideal to reuse the State Enum type. Would the answer in this stackoverflow article help address the import problem? https://stackoverflow.com/questions/66524793/how-to-use-the-enum-of-one-proto-file-in-another-file
> >
> >
> > I've tried it, but there is an issue with backward compatibility
>
> Does the compatibility check also fail if the field is `optional` instead of `required`?
Yes, it does. Backward compatibility check fails for `hdds-interface-server` from which I've tried to pull an enum. Furthermore I tried using `optional` there, but no avail.
--
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 #4443: HDDS-7098. Provide a way for admin to identify all unhealthy container replicas
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #4443:
URL: https://github.com/apache/ozone/pull/4443#issuecomment-1485435790
cc @adoroszlai can you 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