Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on PR #15507: URL: https://github.com/apache/kafka/pull/15507#issuecomment-2001031718 > Do you have any in particular? how about https://issues.apache.org/jira/browse/KAFKA-16376 ? or check the following link: https://issues.apache.org/jira/issues/?jql=project%20%3D%20KAFKA%20AND%20resolution%20%3D%20Unresolved%20AND%20text%20~%20flaky%20ORDER%20BY%20key%20DESC%2C%20priority%20DESC%2C%20updated%20DESC -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on PR #15507: URL: https://github.com/apache/kafka/pull/15507#issuecomment-277204 I will take a look for some unassigned tickets! Do you have any in particular? -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on PR #15507: URL: https://github.com/apache/kafka/pull/15507#issuecomment-1999649048 @ChrisAHolland thanks for contribution. If you have free cycle, please help us fix the flaky. There are a bunch of tickets about flaky tests :) -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 merged PR #15507: URL: https://github.com/apache/kafka/pull/15507 -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on PR #15507: URL: https://github.com/apache/kafka/pull/15507#issuecomment-1998993124 @ChrisAHolland thank for double checking the failed tests. Will merge it later! -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on PR #15507: URL: https://github.com/apache/kafka/pull/15507#issuecomment-1998991332 @chia7712 I believe test failures are orthogonal, it seems all recently merged PR's have been having test failures. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1525616031
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -40,18 +40,22 @@ public static BoundedList newArrayBacked(int
maxLength) {
}
public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
+if (initialCapacity <= 0) {
+throw new IllegalArgumentException("Invalid non-positive
initialCapacity of " + initialCapacity);
+}
return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
}
-public BoundedList(int maxLength, List underlying) {
+private BoundedList(int maxLength, List underlying) {
if (maxLength <= 0) {
throw new IllegalArgumentException("Invalid non-positive maxLength
of " + maxLength);
}
-this.maxLength = maxLength;
+
if (underlying.size() > maxLength) {
throw new BoundedListTooLongException("Cannot wrap list, because
it is longer than " +
-"the maximum length " + maxLength);
+"the maximum length " + maxLength);
Review Comment:
Done, thanks!
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1525585193
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -40,18 +40,22 @@ public static BoundedList newArrayBacked(int
maxLength) {
}
public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
+if (initialCapacity <= 0) {
+throw new IllegalArgumentException("Invalid non-positive
initialCapacity of " + initialCapacity);
+}
return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
}
-public BoundedList(int maxLength, List underlying) {
+private BoundedList(int maxLength, List underlying) {
if (maxLength <= 0) {
throw new IllegalArgumentException("Invalid non-positive maxLength
of " + maxLength);
}
-this.maxLength = maxLength;
+
if (underlying.size() > maxLength) {
throw new BoundedListTooLongException("Cannot wrap list, because
it is longer than " +
-"the maximum length " + maxLength);
+"the maximum length " + maxLength);
Review Comment:
please revert this one, thanks :)
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1525523190
##
server-common/src/test/java/org/apache/kafka/server/mutable/BoundedListTest.java:
##
@@ -165,18 +199,24 @@ public void testIterator() {
@Test
public void testIteratorIsImmutable() {
-BoundedList list = new BoundedList<>(3, new
ArrayList<>(Arrays.asList(1, 2, 3)));
+BoundedList list = BoundedList.newArrayBacked(3);
+list.add(1);
+list.add(2);
+list.add(3);
assertThrows(UnsupportedOperationException.class,
-() -> list.iterator().remove());
+() -> list.iterator().remove());
Review Comment:
@chia7712 Resolved indenting changes
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1525490485
##
server-common/src/test/java/org/apache/kafka/server/mutable/BoundedListTest.java:
##
@@ -165,18 +199,24 @@ public void testIterator() {
@Test
public void testIteratorIsImmutable() {
-BoundedList list = new BoundedList<>(3, new
ArrayList<>(Arrays.asList(1, 2, 3)));
+BoundedList list = BoundedList.newArrayBacked(3);
+list.add(1);
+list.add(2);
+list.add(3);
assertThrows(UnsupportedOperationException.class,
-() -> list.iterator().remove());
+() -> list.iterator().remove());
Review Comment:
ditto
##
server-common/src/test/java/org/apache/kafka/server/mutable/BoundedListTest.java:
##
@@ -165,18 +199,24 @@ public void testIterator() {
@Test
public void testIteratorIsImmutable() {
-BoundedList list = new BoundedList<>(3, new
ArrayList<>(Arrays.asList(1, 2, 3)));
+BoundedList list = BoundedList.newArrayBacked(3);
+list.add(1);
+list.add(2);
+list.add(3);
assertThrows(UnsupportedOperationException.class,
-() -> list.iterator().remove());
+() -> list.iterator().remove());
assertThrows(UnsupportedOperationException.class,
-() -> list.listIterator().remove());
+() -> list.listIterator().remove());
}
@Test
public void testSubList() {
-BoundedList list = new BoundedList<>(3, new
ArrayList<>(Arrays.asList(1, 2, 3)));
+BoundedList list = BoundedList.newArrayBacked(3);
+list.add(1);
+list.add(2);
+list.add(3);
assertEquals(Arrays.asList(2), list.subList(1, 2));
assertThrows(UnsupportedOperationException.class,
-() -> list.subList(1, 2).remove(2));
+() -> list.subList(1, 2).remove(2));
Review Comment:
Could you please remove the indentation?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on PR #15507: URL: https://github.com/apache/kafka/pull/15507#issuecomment-1998410642 @chia7712 Thank you, updated -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1525419354
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -40,18 +40,17 @@ public static BoundedList newArrayBacked(int
maxLength) {
}
public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
+if (initialCapacity <= 0) {
+throw new IllegalArgumentException("Invalid non-positive
initialCapacity of " + initialCapacity);
+}
return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
}
-public BoundedList(int maxLength, List underlying) {
+private BoundedList(int maxLength, List underlying) {
if (maxLength <= 0) {
throw new IllegalArgumentException("Invalid non-positive maxLength
of " + maxLength);
}
this.maxLength = maxLength;
-if (underlying.size() > maxLength) {
Review Comment:
this check is still useful to me since all helpers are based on this
constructor, and so it is a good safety catch.
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -40,18 +40,17 @@ public static BoundedList newArrayBacked(int
maxLength) {
}
public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
+if (initialCapacity <= 0) {
Review Comment:
nice check!
##
metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java:
##
@@ -172,8 +172,7 @@ ControllerResult>
incrementalAlterConfigs(
Map>> configChanges,
boolean newlyCreatedResource
) {
-List outputRecords =
Review Comment:
Could you please revert those changes? the simpler the better :)
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524147955
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
Updated
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524143322
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
Sure, sounds good to me
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524140865
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
> I like the idea of migrating newArrayBacked(int maxLength) to
newLinkedListBacked(int maxLength), this would also help cut down on array list
resizing time, as currently this constructor creates a default array list,
which is size 10
I did not suggest to use Linked list for current usage
My point was keeping the constructor can have flexibility of changing the
list implementation in the future
We need to consider the cost of iteration before adopting the linked list.
To simplify this PR and address the origin propose, changing the scope from
public to private is good enough. WDYT?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on PR #15507: URL: https://github.com/apache/kafka/pull/15507#issuecomment-1996246082 @chia7712 @jolshan Pushed an update based on discussion -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524100301
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
I like the idea of migrating `newArrayBacked(int maxLength`) to
`newLinkedListBacked(int maxLength)`, this would also help cut down on array
list resizing time, as currently this constructor creates a default array list,
which is size 10
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524078277
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
1. maybe `private` is enough. the another view is that we can have
`newListBacked` for more flexible usage in the future, and the implementation
is based on the constructor taking a passed list.
```java
public static BoundedList newListBacked(int maxLength) {
return new BoundedList<>(maxLength, new LinkedList<>());
}
```
2. "ArrayList resizing operations don't seem to be a concern to people." ->
caller can fix it by defining "initialCapacity"
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524075123
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
I think so. It looks like the consensus is:
1. Remove constructor that takes in a list.
2. Keep both versions of `newArrayBacked`, as ArrayList resizing operations
don't seem to be a concern to people.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524073993
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
-return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
-}
-
-public BoundedList(int maxLength, List underlying) {
+public BoundedList(int maxLength) {
if (maxLength <= 0) {
throw new IllegalArgumentException("Invalid non-positive maxLength
of " + maxLength);
}
this.maxLength = maxLength;
-if (underlying.size() > maxLength) {
-throw new BoundedListTooLongException("Cannot wrap list, because
it is longer than " +
-"the maximum length " + maxLength);
-}
-this.underlying = underlying;
+this.underlying = new ArrayList<>(maxLength);
Review Comment:
Got it, so we can keep the existing `newArrayBacked()` constructors and get
rid of the constructor that takes in a list.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
jolshan commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524070820
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
-return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
-}
-
-public BoundedList(int maxLength, List underlying) {
+public BoundedList(int maxLength) {
if (maxLength <= 0) {
throw new IllegalArgumentException("Invalid non-positive maxLength
of " + maxLength);
}
this.maxLength = maxLength;
-if (underlying.size() > maxLength) {
-throw new BoundedListTooLongException("Cannot wrap list, because
it is longer than " +
-"the maximum length " + maxLength);
-}
-this.underlying = underlying;
+this.underlying = new ArrayList<>(maxLength);
Review Comment:
Yeah, it seems like we want to make the constructor private and use the
newArrayBacked methods to accomplish what we want.
I think we can all agree directly passing in the underlying arraylist is not
what we want
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
jolshan commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524069898
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
My understanding here is that we do want to know an expected size to start
out with, but we shouldn't necessarily link it directly to an array we are
passing in as the constructor does. Does that mean the deleted code here is
actually the behavior we want?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524061026
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
-return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
-}
-
-public BoundedList(int maxLength, List underlying) {
+public BoundedList(int maxLength) {
if (maxLength <= 0) {
throw new IllegalArgumentException("Invalid non-positive maxLength
of " + maxLength);
}
this.maxLength = maxLength;
-if (underlying.size() > maxLength) {
-throw new BoundedListTooLongException("Cannot wrap list, because
it is longer than " +
-"the maximum length " + maxLength);
-}
-this.underlying = underlying;
+this.underlying = new ArrayList<>(maxLength);
Review Comment:
Changing the access scope is to fix the issue you described:
> This is problematic because the the constructor created the BoundedList
using a reference to underlying, not a copy of it; therefore, a user could add
elements to underlying instead of their newly instantiated BoundedList and
force the BoundedList to be larger than its maxLength`.
We should avoid it from using in code and so the `private` is good practice.
All callers should use `newArrayBacked` to create `BoundedList`
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1524056640
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
> I'm not sure what you mean, could you please elaborate?
sorry for unclear comment.
It seems to me "initialCapacity" is still useful if we "expect" the size of
batch could be large. For example: create/delete a bunch of topics - that is a
common usage. For such use cases, it would be better to set the initial
capacity to avoid the resize problem you described.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1523909237
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
I'm not sure what you mean, could you please elaborate?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1523907637
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
-return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
-}
-
-public BoundedList(int maxLength, List underlying) {
+public BoundedList(int maxLength) {
if (maxLength <= 0) {
throw new IllegalArgumentException("Invalid non-positive maxLength
of " + maxLength);
}
this.maxLength = maxLength;
-if (underlying.size() > maxLength) {
-throw new BoundedListTooLongException("Cannot wrap list, because
it is longer than " +
-"the maximum length " + maxLength);
-}
-this.underlying = underlying;
+this.underlying = new ArrayList<>(maxLength);
Review Comment:
Sorry I am confused on what you want to change to `private`? Today, passing
in the source list is only used in unit tests, not any source code. If it's
only used for unit tests I think it should be removed so that tests interact
with the BoundedList the same way the source code does.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
chia7712 commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1523656781
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
-return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
-}
-
-public BoundedList(int maxLength, List underlying) {
+public BoundedList(int maxLength) {
if (maxLength <= 0) {
throw new IllegalArgumentException("Invalid non-positive maxLength
of " + maxLength);
}
this.maxLength = maxLength;
-if (underlying.size() > maxLength) {
-throw new BoundedListTooLongException("Cannot wrap list, because
it is longer than " +
-"the maximum length " + maxLength);
-}
-this.underlying = underlying;
+this.underlying = new ArrayList<>(maxLength);
Review Comment:
It seems to me the large batch operation is not usual. It could be a
potential performance regression if we always create `ArrayList` with `1`
initial size.
If the origin constructor has potential issue about using a reference to
`underlying`, we can change it from `public` to `private`.
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
The operation which we can ensure the size of batch before processing can
use this method.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on PR #15507: URL: https://github.com/apache/kafka/pull/15507#issuecomment-1994913616 @showuon @jolshan Would appreciate if you could 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1521727181
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
Looking at the usage here, I can't see a reason why this would be desirable,
maybe the intention was to have the option to save on same space until
absolutely necessary? Although, as I mentioned in the PR description, this is
not worth the resizing costs. Also, this method is guaranteed to add more
records to the list than `ids.size()` as the method add elements in multiple
places.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1521713491
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
-return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
-}
-
-public BoundedList(int maxLength, List underlying) {
Review Comment:
I considered this at first, but then I realized that this is only ever used
in unit tests. Due to that fact, I decided to remove it and have he unit tests
match how the class is used in the source code.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
cadonna commented on code in PR #15507:
URL: https://github.com/apache/kafka/pull/15507#discussion_r1521177424
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
Review Comment:
I see one usage of this method and I suspect there is a reason the author
did not use `newArrayBacked(MAX_RECORDS_PER_USER_OP)`.
##
server-common/src/main/java/org/apache/kafka/server/mutable/BoundedList.java:
##
@@ -35,24 +35,12 @@ public class BoundedList implements List {
private final int maxLength;
private final List underlying;
-public static BoundedList newArrayBacked(int maxLength) {
-return new BoundedList<>(maxLength, new ArrayList<>());
-}
-
-public static BoundedList newArrayBacked(int maxLength, int
initialCapacity) {
-return new BoundedList<>(maxLength, new ArrayList<>(initialCapacity));
-}
-
-public BoundedList(int maxLength, List underlying) {
Review Comment:
Wouldn't it be less intrusive to just make a copy of `underlying` instead of
removing the whole constructor?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup BoundedList to Make Constructors More Safe [kafka]
ChrisAHolland commented on PR #15507: URL: https://github.com/apache/kafka/pull/15507#issuecomment-1990325002 @chia7712 @cadonna Hi, saw you approved some recent PR's, wondering if you could take a look at mine or suggest someone better suited that I can ask? Thanks! -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
