Skip to content

MINOR: Cleanups in Shell Module #16204

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.

Sign up for GitHub

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

Merged
chia7712 merged 1 commit into from
Jun 5, 2024
Merged

MINOR: Cleanups in Shell Module #16204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shell/src/main/java/org/apache/kafka/shell/MetadataShell.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ static File parentParent(File file) {
* Take the FileLock in the given directory, if it already exists. Technically, there is a
* TOCTOU bug here where someone could create and lock the lockfile in between our check
* and our use. However, this is very unlikely to ever be a problem in practice, and closing
* this hole would require the parent parent directory to always be writable when loading a
* this hole would require the parent directory to always be writable when loading a
* snapshot so that we could create our .lock file there.
*/
static FileLock takeDirectoryLockIfExists(File directory) {
Expand Down
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/CatCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,6 @@ public int hashCode() {
public boolean equals(Object other) {
if (!(other instanceof CatCommandHandler)) return false;
CatCommandHandler o = (CatCommandHandler) other;
if (!Objects.equals(o.targets, targets)) return false;
return true;
return Objects.equals(o.targets, targets);
}
}
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/CdCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ public int hashCode() {
public boolean equals(Object other) {
if (!(other instanceof CdCommandHandler)) return false;
CdCommandHandler o = (CdCommandHandler) other;
if (!o.target.equals(target)) return false;
return true;
return o.target.equals(target);
}
}
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/ErroneousCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ public int hashCode() {
public boolean equals(Object other) {
if (!(other instanceof ErroneousCommandHandler)) return false;
ErroneousCommandHandler o = (ErroneousCommandHandler) other;
if (!Objects.equals(o.message, message)) return false;
return true;
return Objects.equals(o.message, message);
}

@Override
Expand Down
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/ExitCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ public int hashCode() {

@Override
public boolean equals(Object other) {
if (!(other instanceof ExitCommandHandler)) return false;
return true;
return other instanceof ExitCommandHandler;
}
}
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/FindCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ public int hashCode() {
public boolean equals(Object other) {
if (!(other instanceof FindCommandHandler)) return false;
FindCommandHandler o = (FindCommandHandler) other;
if (!Objects.equals(o.paths, paths)) return false;
return true;
return Objects.equals(o.paths, paths);
}
}
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/HelpCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ public int hashCode() {

@Override
public boolean equals(Object other) {
if (!(other instanceof HelpCommandHandler)) return false;
return true;
return other instanceof HelpCommandHandler;
}
}
12 changes: 4 additions & 8 deletions shell/src/main/java/org/apache/kafka/shell/command/LsCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,7 @@ public void run(
MetadataNodeInfo info = entryOption.get();
MetadataNode node = info.node();
if (node.isDirectory()) {
List<String> children = new ArrayList<>();
children.addAll(node.childNames());
List<String> children = new ArrayList<>(node.childNames());
children.sort(String::compareTo);
targetDirectories.add(
new TargetDirectory(info.lastPathComponent(), children));
Expand All @@ -128,8 +127,7 @@ public void run(
}
}));
}
OptionalInt screenWidth = shell.isPresent() ?
OptionalInt.of(shell.get().screenWidth()) : OptionalInt.empty();
OptionalInt screenWidth = shell.map(interactiveShell -> OptionalInt.of(interactiveShell.screenWidth())).orElseGet(OptionalInt::empty);
log.trace("LS : targetFiles = {}, targetDirectories = {}, screenWidth = {}",
targetFiles, targetDirectories, screenWidth);
printTargets(writer, screenWidth, targetFiles, targetDirectories);
Expand Down Expand Up @@ -271,8 +269,7 @@ public boolean equals(Object o) {
if (!(o instanceof ColumnSchema)) return false;
ColumnSchema other = (ColumnSchema) o;
if (entriesPerColumn != other.entriesPerColumn) return false;
if (!Arrays.equals(columnWidths, other.columnWidths)) return false;
return true;
return Arrays.equals(columnWidths, other.columnWidths);
}

@Override
Expand All @@ -298,7 +295,6 @@ public int hashCode() {
public boolean equals(Object other) {
if (!(other instanceof LsCommandHandler)) return false;
LsCommandHandler o = (LsCommandHandler) other;
if (!Objects.equals(o.targets, targets)) return false;
return true;
return Objects.equals(o.targets, targets);
}
}
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/ManCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ public int hashCode() {
public boolean equals(Object other) {
if (!(other instanceof ManCommandHandler)) return false;
ManCommandHandler o = (ManCommandHandler) other;
if (!o.cmd.equals(cmd)) return false;
return true;
return o.cmd.equals(cmd);
}
}
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/NoOpCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ public int hashCode() {

@Override
public boolean equals(Object other) {
if (!(other instanceof NoOpCommandHandler)) return false;
return true;
return other instanceof NoOpCommandHandler;
}
}
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/PwdCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ public int hashCode() {

@Override
public boolean equals(Object other) {
if (!(other instanceof PwdCommandHandler)) return false;
return true;
return other instanceof PwdCommandHandler;
}
}
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/command/TreeCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ public int hashCode() {
public boolean equals(Object other) {
if (!(other instanceof TreeCommandHandler)) return false;
TreeCommandHandler o = (TreeCommandHandler) other;
if (!Objects.equals(o.targets, targets)) return false;
return true;
return Objects.equals(o.targets, targets);
}
}
3 changes: 1 addition & 2 deletions shell/src/main/java/org/apache/kafka/shell/glob/GlobVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ public boolean equals(Object o) {
if (!(o instanceof MetadataNodeInfo)) return false;
MetadataNodeInfo other = (MetadataNodeInfo) o;
if (!Arrays.equals(path, other.path)) return false;
if (!node.equals(other.node)) return false;
return true;
return node.equals(other.node);
}

@Override
Expand Down
18 changes: 9 additions & 9 deletions shell/src/test/java/org/apache/kafka/shell/command/CommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,39 +30,39 @@
public class CommandTest {
@Test
public void testParseCommands() {
assertEquals(new CatCommandHandler(Arrays.asList("foo")),
assertEquals(new CatCommandHandler(Collections.singletonList("foo")),
new Commands(true).parseCommand(Arrays.asList("cat", "foo")));
assertEquals(new CdCommandHandler(Optional.empty()),
new Commands(true).parseCommand(Arrays.asList("cd")));
new Commands(true).parseCommand(Collections.singletonList("cd")));
assertEquals(new CdCommandHandler(Optional.of("foo")),
new Commands(true).parseCommand(Arrays.asList("cd", "foo")));
assertEquals(new ExitCommandHandler(),
new Commands(true).parseCommand(Arrays.asList("exit")));
new Commands(true).parseCommand(Collections.singletonList("exit")));
assertEquals(new HelpCommandHandler(),
new Commands(true).parseCommand(Arrays.asList("help")));
new Commands(true).parseCommand(Collections.singletonList("help")));
assertEquals(new HistoryCommandHandler(3),
new Commands(true).parseCommand(Arrays.asList("history", "3")));
assertEquals(new HistoryCommandHandler(Integer.MAX_VALUE),
new Commands(true).parseCommand(Arrays.asList("history")));
new Commands(true).parseCommand(Collections.singletonList("history")));
assertEquals(new LsCommandHandler(Collections.emptyList()),
new Commands(true).parseCommand(Arrays.asList("ls")));
new Commands(true).parseCommand(Collections.singletonList("ls")));
assertEquals(new LsCommandHandler(Arrays.asList("abc", "123")),
new Commands(true).parseCommand(Arrays.asList("ls", "abc", "123")));
assertEquals(new PwdCommandHandler(),
new Commands(true).parseCommand(Arrays.asList("pwd")));
new Commands(true).parseCommand(Collections.singletonList("pwd")));
}

@Test
public void testParseInvalidCommand() {
assertEquals(new ErroneousCommandHandler("invalid choice: 'blah' (choose " +
"from 'cat', 'cd', 'exit', 'find', 'help', 'history', 'ls', 'man', 'pwd', 'tree')"),
new Commands(true).parseCommand(Arrays.asList("blah")));
new Commands(true).parseCommand(Collections.singletonList("blah")));
}

@Test
public void testEmptyCommandLine() {
assertEquals(new NoOpCommandHandler(),
new Commands(true).parseCommand(Arrays.asList("")));
new Commands(true).parseCommand(Collections.singletonList("")));
assertEquals(new NoOpCommandHandler(),
new Commands(true).parseCommand(Collections.emptyList()));
}
Expand Down
13 changes: 7 additions & 6 deletions shell/src/test/java/org/apache/kafka/shell/glob/GlobVisitorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -99,11 +100,11 @@ public MetadataNode child(String name) {
}

static class InfoConsumer implements Consumer<Optional<MetadataNodeInfo>> {
private Optional<List<MetadataNodeInfo>> infos = null;
private Optional<List<MetadataNodeInfo>> infos = Optional.empty();
Copy link
Copy Markdown
Member

@chia7712 chia7712 Jun 5, 2024

Choose a reason for hiding this comment

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

not sure why we need Optional ... it seems empty collection can be equal to Optional.empty

Copy link
Copy Markdown
Collaborator Author

@sjhajharia sjhajharia Jun 5, 2024

Choose a reason for hiding this comment

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

I made that change just for consistency with the variable declaration. We have it defined as Optional and I felt Optional.empty() made more sense than null here.
Even the IDE complains here: Null is used for 'Optional' type in declaration.
lmk if it makes sense here.
Thanks!


@Override
public void accept(Optional<MetadataNodeInfo> info) {
if (infos == null) {
if (!infos.isPresent()) {
if (info.isPresent()) {
infos = Optional.of(new ArrayList<>());
infos.get().add(info.get());
Expand Down Expand Up @@ -137,7 +138,7 @@ public void testDotDot() {
InfoConsumer consumer = new InfoConsumer();
GlobVisitor visitor = new GlobVisitor("..", consumer);
visitor.accept(DATA);
assertEquals(Optional.of(Arrays.asList(
assertEquals(Optional.of(Collections.singletonList(
new MetadataNodeInfo(new String[0], DATA.root()))), consumer.infos);
}

Expand All @@ -146,7 +147,7 @@ public void testDoubleDotDot() {
InfoConsumer consumer = new InfoConsumer();
GlobVisitor visitor = new GlobVisitor("../..", consumer);
visitor.accept(DATA);
assertEquals(Optional.of(Arrays.asList(
assertEquals(Optional.of(Collections.singletonList(
new MetadataNodeInfo(new String[0], DATA.root()))), consumer.infos);
}

Expand Down Expand Up @@ -189,8 +190,8 @@ public void testAbsoluteGlob() {
InfoConsumer consumer = new InfoConsumer();
GlobVisitor visitor = new GlobVisitor("/a?pha", consumer);
visitor.accept(DATA);
assertEquals(Optional.of(Arrays.asList(
new MetadataNodeInfo(new String[] {"alpha"},
assertEquals(Optional.of(Collections.singletonList(
new MetadataNodeInfo(new String[]{"alpha"},
DATA.root().child("alpha")))), consumer.infos);
}
}