Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Worker.ActiveClients is negative when load from ufs #16784

Merged

Conversation

flaming-archer
Copy link
Contributor

What changes are proposed in this pull request?

Fix Worker.ActiveClients is negative when load from ufs

Why are the changes needed?

Please clarify why the changes are needed. For instance,
createUfsBlockReader will invoke closeUfsBlock then invoke commitBlock then this metric decrease.
But forget to increase firstly . So it need to increase firstly.

Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
no.

@flaming-archer
Copy link
Contributor Author

Maybe I can find why Worker.ActiveClients is a big int too. I'll try.

@ssz1997
Copy link
Contributor

ssz1997 commented Jan 25, 2023

@flaming-archer Are you still trying to figure out something, or this is ready to be reviewed?

@flaming-archer
Copy link
Contributor Author

@flaming-archer Are you still trying to figure out something, or this is ready to be reviewed?

Sorry for later reply cause of CNY. Yes, I think this is ready to be reviewed. This method will be invoked by download command. And will decrease Worker.ActiveClients, but not increase firstly.

@ssz1997 ssz1997 requested a review from jja725 January 30, 2023 18:40
Copy link
Contributor

@jja725 jja725 left a comment

Choose a reason for hiding this comment

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

LGTM

@flaming-archer
Copy link
Contributor Author

Can we merge this pr now, hhh @jja725 @ssz1997 ?

Copy link
Contributor

@maobaolong maobaolong left a comment

Choose a reason for hiding this comment

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

LGTM. These kind improvements are always welcome.

@maobaolong
Copy link
Contributor

@flaming-archer @jja725 Just a question, is there something wrong if some clients didn't close the reader normally? Will the Active client metric show the wrong value?

@jja725
Copy link
Contributor

jja725 commented Feb 13, 2023

@flaming-archer @jja725 Just a question, is there something wrong if some clients didn't close the reader normally? Will the Active client metric show the wrong value?

yes it would show wrong value in your case. Maybe we can have another PR to fix that.

@flaming-archer
Copy link
Contributor Author

In our environment, 'Worker.ActiveClients' is often a large value. I think that to fix this problem, we maybe can refer to the design of audit log to unify the entrance and exit. @flaming-archer @jja725

@flaming-archer
Copy link
Contributor Author

I think this can be merged.

@Xenorith
Copy link
Contributor

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit 60d5156 into Alluxio:master Feb 15, 2023
YangchenYe323 pushed a commit to YangchenYe323/alluxio that referenced this pull request Apr 16, 2023
### What changes are proposed in this pull request?

Fix Worker.ActiveClients is negative when load from ufs
### Why are the changes needed?

Please clarify why the changes are needed. For instance,
createUfsBlockReader will invoke closeUfsBlock then invoke commitBlock
then this metric decrease.
But forget to increase firstly . So it need to increase firstly.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
no.

pr-link: Alluxio#16784
change-id: cid-13d0875dc42336cc612dfed121bba8572164ed60
jiacheliu3 pushed a commit to jiacheliu3/alluxio that referenced this pull request May 16, 2023
### What changes are proposed in this pull request?

Fix Worker.ActiveClients is negative when load from ufs
### Why are the changes needed?

Please clarify why the changes are needed. For instance,
createUfsBlockReader will invoke closeUfsBlock then invoke commitBlock
then this metric decrease.
But forget to increase firstly . So it need to increase firstly.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
no.

pr-link: Alluxio#16784
change-id: cid-13d0875dc42336cc612dfed121bba8572164ed60
jiacheliu3 pushed a commit to jiacheliu3/alluxio that referenced this pull request May 16, 2023
### What changes are proposed in this pull request?

Fix Worker.ActiveClients is negative when load from ufs
### Why are the changes needed?

Please clarify why the changes are needed. For instance,
createUfsBlockReader will invoke closeUfsBlock then invoke commitBlock
then this metric decrease.
But forget to increase firstly . So it need to increase firstly.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
no.

pr-link: Alluxio#16784
change-id: cid-13d0875dc42336cc612dfed121bba8572164ed60
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants