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

feat(rest server): add warnings when listing fails with kafka error #3069

Merged

Conversation

@rodesai
Copy link
Contributor

commented Jul 12, 2019

This patch adds a mechanism for returning warnings in KsqlEntity for
successful api responses. This patch also adds some warnings for source
listings. Now, when a client tries to list or describe a source, and ksql
is unable to get the details of the backing topic from kafka, a result is
returned that includes 0-values for the underlying topic info (partitions,
replicas), and includes response warnings about the issue with Kafka.

@rodesai rodesai requested a review from confluentinc/ksql as a code owner Jul 12, 2019

@rodesai rodesai force-pushed the rodesai:add-ksql-warnings-for-missing-topic branch 2 times, most recently from 256103f to 37949bd Jul 12, 2019

@big-andy-coates
Copy link
Member

left a comment

LGTM @rodesai, just a few nits / suggestions.

final ConfiguredStatement<?> statement,
final KsqlExecutionContext executionContext,
final ServiceContext serviceContext,
final List<? extends StructuredDataSource<?>> sources

This comment has been minimized.

Copy link
@big-andy-coates

big-andy-coates Jul 12, 2019

Member

Just use the interface. StructuredDataSource is an implementation detail only.

Suggested change
final List<? extends StructuredDataSource<?>> sources
final List<? extends DataSource<?>> sources

private void assertSourceListWithWarning(
final KsqlEntity entity,
final StructuredDataSource<?>... sources) {

This comment has been minimized.

Copy link
@big-andy-coates

big-andy-coates Jul 12, 2019

Member
Suggested change
final StructuredDataSource<?>... sources) {
final DataSource<?>... sources) {
@@ -24,7 +24,7 @@
import io.confluent.ksql.util.timestamp.TimestampExtractionPolicy;

@Immutable
abstract class StructuredDataSource<K> implements DataSource<K> {
public abstract class StructuredDataSource<K> implements DataSource<K> {

This comment has been minimized.

Copy link
@big-andy-coates

big-andy-coates Jul 12, 2019

Member

Having this package private is a deliberate thing. It's an implementation detail. As such no other code should rely on it. Just use the interface.

Suggested change
public abstract class StructuredDataSource<K> implements DataSource<K> {
abstract class StructuredDataSource<K> implements DataSource<K> {

This comment has been minimized.

Copy link
@rodesai

rodesai Jul 12, 2019

Author Contributor

Yes this makes perfect sense. Still getting used to the refactor.

this.statementText = statementText;
this.warnings = warnings == null ? Collections.emptyList() : warnings;

This comment has been minimized.

Copy link
@big-andy-coates

big-andy-coates Jul 12, 2019

Member

Take a defensive immutable copy:

Suggested change
this.warnings = warnings == null ? Collections.emptyList() : warnings;
this.warnings = warnings == null ? Collections.emptyList() : ImmutableList.copyOf(warnings);

@JsonCreator
public KsqlWarning(@JsonProperty("message") final String message) {
this.message = message;

This comment has been minimized.

Copy link
@big-andy-coates

big-andy-coates Jul 12, 2019

Member

null check?

Suggested change
this.message = message;
this.message = Objects.requireNotNull(message, "message");
dataSource.getKafkaTopicName()
) : 0
)
topicDescription != null ? topicDescription.partitions().size() : 0,

This comment has been minimized.

Copy link
@big-andy-coates

big-andy-coates Jul 12, 2019

Member

Is it ever null? This is only called from our code, right? Consider either throwing an NPE or if null is valid, then make this explicit in the interface by making it Optional< TopicDescription>

This comment has been minimized.

Copy link
@rodesai

rodesai Jul 14, 2019

Author Contributor

It's null if the description is not extended, or if we couldn't get the description.


// CHECKSTYLE_RULES.OFF: ClassDataAbstractionCoupling
public final class ListSourceExecutor {
// CHECKSTYLE_RULES.ON: ClassDataAbstractionCoupling

private ListSourceExecutor() { }

private static Optional<KsqlEntity> sourceDescriptionList(

This comment has been minimized.

Copy link
@big-andy-coates

big-andy-coates Jul 12, 2019

Member

super nit: private static to the bottom of the class file?

private SourceDescriptionWithWarnings(
final List<KsqlWarning> warnings,
final SourceDescription description) {
this.warnings = warnings;

This comment has been minimized.

Copy link
@big-andy-coates

big-andy-coates Jul 12, 2019

Member

I know its internal, but I'd still null check these as it makes the code easier to grok, i.e. you know the field is never null. (Nulls are evil).

@big-andy-coates big-andy-coates requested review from confluentinc/ksql, spena, vcrfxia, hjafarpour and agavra and removed request for confluentinc/ksql Jul 12, 2019

@rodesai rodesai changed the base branch from master to 5.3.x Jul 12, 2019

feat[rest server]: add warnings when listing fails with kafka error
This patch adds a mechanism for returning warnings in KsqlEntity for
successful api responses. This patch also adds some warnings for source
listings. Now, when a client tries to list or describe a source, and ksql
is unable to get the details of the backing topic from kafka, a result is
returned that includes 0-values for the underlying topic info (partitions,
replicas), and includes response warnings about the issue with Kafka.

@rodesai rodesai force-pushed the rodesai:add-ksql-warnings-for-missing-topic branch from 37949bd to e35846d Jul 14, 2019

@rodesai rodesai requested a review from JimGalasyn as a code owner Jul 14, 2019

@vcrfxia vcrfxia changed the title feat[rest server]: add warnings when listing fails with kafka error feat(rest server): add warnings when listing fails with kafka error Jul 15, 2019

@agavra
agavra approved these changes Jul 18, 2019

@agavra agavra requested a review from confluentinc/ksql Jul 18, 2019

@big-andy-coates
Copy link
Member

left a comment

LGTM

@JimGalasyn
Copy link
Member

left a comment

LGTM

@rodesai rodesai merged commit 7647e05 into confluentinc:5.3.x Jul 23, 2019

2 checks passed

Semantic Pull Request ready to be squashed
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.