Skip to content

MINOR: Update usage of deprecated API #6146

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
mjsax merged 4 commits into from
Jan 29, 2019
Merged

MINOR: Update usage of deprecated API #6146

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0881366
MINOR: Update usage of deprecated API
mjsax Jan 15, 2019
631115c
Github comments
mjsax Jan 22, 2019
40fc3a2
Github comments
mjsax Jan 24, 2019
d0a8b53
Fixed spotbugs error
mjsax Jan 26, 2019
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 core/src/main/scala/kafka/tools/StreamsResetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public int run(final String[] args,
e.printStackTrace(System.err);
} finally {
if (kafkaAdminClient != null) {
kafkaAdminClient.close(60, TimeUnit.SECONDS);
kafkaAdminClient.close(java.time.Duration.ofSeconds(60));

@mjsax mjsax Jan 15, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We cannot import java.time.Duration because we already import javax.xml.datatype.Duration.

I was wondering if we could get rid of javax.xml.datatype.Duration though and replace it with java.time.Duration that was not available in Java7 when we added javax.xml.datatype.Duration.

Thoughts? \cc @jeqo who extended the reset tool via #3831

@guozhangwang guozhangwang Jan 15, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The only place we need this duration is

duration.negate().addTo(now);

java.time.Duration has negated but its addTo is taking a Temporal, and all its extends like LocalTime etc do not have the right API of getTime. So I think we just keep it like this.

@ijuma ijuma Jan 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should remove the javax.xml.datatype.Duration usage since it's not part of the Java base module.

@mjsax mjsax Jan 22, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was locking into the corresponding KIPs

Both say:

--by-duration  PnDTnHnMnS

Duration must be specified in ISO8601 format.

Looking into JavaDocs of java.time.Duration#parse() it says:

     * Obtains a {@code Duration} from a text string such as {@code PnDTnHnMn.nS}.
     * <p>
     * This will parse a textual representation of a duration, including the
     * string produced by {@code toString()}. The formats accepted are based
     * on the ISO-8601 duration format {@code PnDTnHnMn.nS} with days
     * considered to be exactly 24 hours.

Thus, it seems save to replace

final javax.xml.datatype.Duration durationParsed = DatatypeFactory.newInstance().newDuration(duration);

with

final java.time.Duration durationParsed = java.time.Duration.parse(duration);

Btw: we should do the same for kafka.admin.ConsumerGroupCommand (I can add this change to this PR).

Thoughts?

@jeqo jeqo Jan 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mjsax LGTM

Both kafka/tools/StreamsResetter.java and kafka/admin/ConsumerGroupCommand.scala should be migrated to remove usage of javax.xml.datatype.*

@vvcephei vvcephei Jan 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mjsax , I agree, it seems safe and desirable to swap them out.

}
}

Expand Down