Skip to content

Prohibit LinkedList#6112

Merged
jon-wei merged 8 commits intoapache:masterfrom
metamx:prohibit-linked-list
Sep 14, 2018
Merged

Prohibit LinkedList#6112
jon-wei merged 8 commits intoapache:masterfrom
metamx:prohibit-linked-list

Conversation

@leventov
Copy link
Copy Markdown
Member

@leventov leventov commented Aug 6, 2018

All use cases for LinkedList are covered by ArrayList and ArrayDeque, which are better options.

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Aug 8, 2018

👎 I don't see why this refactor is required or the benefits it provides

@leventov
Copy link
Copy Markdown
Member Author

leventov commented Aug 8, 2018

@fjy it protects the codebase from introducing suboptimal code due to inattention or ignorance. It seems to me that there are no such cases in the codebase right now (all places where LinkedList is removed seem to not be important for efficiency), but this is a lucky accident.

Many refactorings are not required but they make supporting the codebase easier.

@leventov
Copy link
Copy Markdown
Member Author

@fjy could you please respond

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Aug 24, 2018

@fjy is your 👎 emoji intended as a "veto" in the ASF sense (https://www.apache.org/foundation/voting.html)?

A code-modification proposal may be stopped dead in its tracks by a -1 vote by a qualified voter. This constitutes a veto, and it cannot be overruled nor overridden by anyone. Vetos stand until and unless withdrawn by their casters.

To prevent vetos from being used capriciously, they must be accompanied by a technical justification showing why the change is bad (opens a security exposure, negatively affects performance, etc. ). A veto without a justification is invalid and has no weight.

If so, please lay out the technical justification as to why you are vetoing this change. If not, please make it clear that you did not actually intend to veto, and this change can go forward if someone else +1s it.

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Aug 29, 2018

I'll change my vote to 0-. I'm not going veto the PR but I also see almost no benefit in it either.

@jon-wei jon-wei merged commit d50b69e into apache:master Sep 14, 2018
@leventov leventov deleted the prohibit-linked-list branch September 14, 2018 05:36
@leventov
Copy link
Copy Markdown
Member Author

@jon-wei thanks

@dclim dclim added this to the 0.13.0 milestone Oct 9, 2018
@jihoonson jihoonson mentioned this pull request Nov 8, 2019
9 tasks
@SEKIRO-J
Copy link
Copy Markdown
Contributor

SEKIRO-J commented Nov 8, 2019

@leventov Sometime we may want to both operate on null values and O(1) pop, neither ArrayList nor ArrayDeque can do it.

@jihoonson
Copy link
Copy Markdown
Contributor

@SEKIRO-J the thing is, iterating on the linked list is expensive because it's iterating through pointers. Even though the time complexity of remove() is O(1), iteration is the most popular operation for the list. I think prohibiting using LinkedList by default is to make you sure that you really need to use it and what is the benefit of using it.

@SEKIRO-J
Copy link
Copy Markdown
Contributor

SEKIRO-J commented Nov 8, 2019

@jihoonson gotcha

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.

7 participants