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

Use arrays to initialize the member variables of MultilevelSplitQueue #14139

Merged
merged 2 commits into from
Nov 1, 2022

Conversation

tangjiangling
Copy link
Member

Previously in MultilevelSplitQueue, levelScheduledTime and
levelMinPriority were initialized using arrays, while
levelWaitingSplits and selectedLevelCounters were initialized using
List, this commit unifies the style by making all these variables
initialized using array style.

Release notes

(x) This is not user-visible and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Sep 15, 2022
@tangjiangling tangjiangling added the no-release-notes This pull request does not require release notes entry label Sep 15, 2022
@hashhar hashhar requested review from findepi, sopel39 and arhimondr and removed request for wendigo October 13, 2022 08:19
@hashhar
Copy link
Member

hashhar commented Oct 28, 2022

@tangjiangling Can you rebase to make sure CI results are fresh and I can merge this then.

Previously in `MultilevelSplitQueue`, `levelScheduledTime` and
`levelMinPriority` were initialized using arrays, while
`levelWaitingSplits` and `selectedLevelCounters` were initialized using
`List`, this commit unifies the style by making all these variables
initialized using array style.
The indexes of these arrays are all "level" related, so we can rename
them.
@tangjiangling
Copy link
Member Author

Rebased from master.

@martint
Copy link
Member

martint commented Oct 28, 2022

Unless there’s a compelling reason to use arrays (measurable performance improvements, memory, etc), we should be using lists.

@hashhar
Copy link
Member

hashhar commented Oct 28, 2022

In this specific case there's no benefit to using lists as far as I can tell. And note that all other code there already uses arrays. I think this change brings some consistency without any additional cost.

@tangjiangling
Copy link
Member Author

Agreed, we have only a small number of elements (5):

  • In terms of performance or resource usage, there is little difference between using array or list.
  • In terms of ease of use, we can consider using list, as it has a richer api

The main purpose of this PR is to unify the types of variables, so if I understand you correctly, you agree that I unify them into lists, right? @martint

@martint
Copy link
Member

martint commented Oct 28, 2022

Unifying them makes sense (either to lists or arrays, as appropriate). I'd like to understand why they are different, though (it may require going through the history of changes)

@tangjiangling
Copy link
Member Author

I'd like to understand why they are different, though (it may require going through the history of changes)

Following your suggestion, I have checked the change history:

d743fb0 introduces the following 3 variables:

  • levelWaitingSplits : List
  • levelMinPriority : AtomicLong[]
  • selectedLevelCounters : List

c05e081 introduces the following 1 variable:

  • levelScheduledTime : AtomicLong[]

From the change history, I think there is no special reason to choose list or array, maybe just the author's development style at that time

So I'll keep this PR as it is, and you can double-check to see if I'm missing something.

@hashhar
Copy link
Member

hashhar commented Oct 31, 2022

@martint This is ok to merge, correct?

EDIT: I assume yes by your reaction to the previous comment. Merging, feel free to do a post-merge review if needed.

@hashhar hashhar merged commit 4f5bc7c into trinodb:master Nov 1, 2022
@github-actions github-actions bot added this to the 402 milestone Nov 1, 2022
@tangjiangling tangjiangling deleted the apply-array-style branch November 1, 2022 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

4 participants