PROD-39429 PART-2 Make REST API(/channels/status) call which acts as Channel Exi… #737

sfc-gh-japatel · 2023-10-26T05:45:44Z

…stence Checker

What

A skeleton code, to find out whether a Streaming Channel exists for a table.
This is a backdoor way to get Channel Status (If client sequencers match) and to find out if a channel exists without opening a channel or without needing to have a StreamingIngestChannel Object
It has a lot of repeated code from Ingest SDK.
We would want to move away from KC code to ingest SDK. More on that will come later SNOW-951262

Why

We could have always opened a new Channel, find offsetToken and assign it to oldChannel.
But this has a risk of opening a new channel everytime and we would want to go away from that. This could also have a lot of channels on table and we could reach a limit.
Doing show channels iliike '<channelName>' in table <tableName> is an expensive operation and is not optimal and also doesnt get away from opening a channel.
This will also help us to have our future code (PR no 3, more on it later) backward compatible.

If interested, take a look at this algorithm: (Internal employees only)

Tests

Added tests.

…stence Checker

sfc-gh-tzhang · 2023-10-27T00:49:54Z

.../java/com/snowflake/kafka/connector/internal/streaming/StreamingChannelExistenceChecker.java

+  private static final KCLogger LOGGER =
+      new KCLogger(StreamingChannelExistenceChecker.class.getName());
+
+  private static final String DEBUG_PREFIX = "Channel Existence Checker - Kafka Connector";


Suggested change

private static final String DEBUG_PREFIX = "Channel Existence Checker - Kafka Connector";

private static final String DEBUG_PREFIX = "StreamingChannelExistenceChecker";

sfc-gh-tzhang · 2023-10-27T00:53:28Z

.../java/com/snowflake/kafka/connector/internal/streaming/StreamingChannelExistenceChecker.java

+
+    try {
+      ObjectMapper objectMapper = new ObjectMapper();
+      objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);


what is this?

good catch, not needed!

sfc-gh-tzhang · 2023-10-27T00:54:00Z

.../java/com/snowflake/kafka/connector/internal/streaming/StreamingChannelExistenceChecker.java

+
+      ChannelExistenceCheckerResponse channelExistenceCheckerResponse;
+      try (CloseableHttpResponse closeableHttpResponse = httpClient.execute(request)) {
+


nit: extra line

sfc-gh-tzhang · 2023-10-27T00:54:48Z

.../java/com/snowflake/kafka/connector/internal/streaming/StreamingChannelExistenceChecker.java

+          LOGGER.warn(
+              "Null response obtained in {} for channel:{}", CHANNEL_STATUS_ENDPOINT, channelName);


why we need a log if we're going to throw?

to know which channel name had this issue.

sfc-gh-tzhang · 2023-10-27T00:56:30Z

.../java/com/snowflake/kafka/connector/internal/streaming/StreamingChannelExistenceChecker.java

+                .equals(SUCCESS.getStatusCode())) {
+          // We check two status codes, one for invalid client sequencers and one for a valid client
+          // Sequencer
+          LOGGER.debug(String.format("%s Channel:%s does exist", DEBUG_PREFIX, channelName));


duplicate with above?

it's Does and doesn't for channel.

sfc-gh-tzhang · 2023-10-27T00:57:13Z

.../java/com/snowflake/kafka/connector/internal/streaming/StreamingChannelExistenceChecker.java

+          "{} Error Getting a valid response for ChannelName:{} error:{}",
+          DEBUG_PREFIX,
+          channelName,
+          e.getMessage());


any issue with returning here instead of thrown an error?

I think that's ok, being unable to get a channel basically means it doesnt exist. We should add that behavior to the javadoc though: @return True if it exists, false if it doesn't or failed to get status

sfc-gh-rcheng · 2023-10-27T18:12:21Z

.../java/com/snowflake/kafka/connector/internal/streaming/StreamingChannelExistenceChecker.java

+   * @param channelName Name of Channel to Check for Existence
+   * @return True if it exists, false if it doesn't.
+   */
+  public boolean checkChannelExistence(final String channelName) {


nit: checkChannelExists so that it's easier to intuit that returning TRUE means the channel exists

makes sense, will do!

sfc-gh-rcheng · 2023-10-27T18:15:05Z

.../java/com/snowflake/kafka/connector/internal/streaming/StreamingChannelExistenceChecker.java

+          "{} Error Getting a valid response for ChannelName:{} error:{}",
+          DEBUG_PREFIX,
+          channelName,
+          e.getMessage());


I think that's ok, being unable to get a channel basically means it doesnt exist. We should add that behavior to the javadoc though: @return True if it exists, false if it doesn't or failed to get status

sfc-gh-rcheng · 2023-10-27T18:19:47Z

.../java/com/snowflake/kafka/connector/internal/streaming/StreamingChannelExistenceChecker.java

+      switch (statusLine.getStatusCode()) {
+          // If we have a 503, BACKOFF
+        case HttpStatus.SC_SERVICE_UNAVAILABLE:
+          throw new BackOffException();


Do we want to retry for this BackOffException? We don't retry in checkChannelExistance

good catch, I copied this function from sdk and I do see we dont have retries in channelexistence, I plan to add retries, but let me check this..

sfc-gh-japatel requested review from sfc-gh-tzhang, sfc-gh-tjones, sfc-gh-rcheng and a team as code owners October 26, 2023 05:45

sfc-gh-japatel changed the title ~~PART-2 Make REST API(/channels/status) call which acts as Channel Exi…~~ PROD-39429 PART-2 Make REST API(/channels/status) call which acts as Channel Exi… Oct 26, 2023

sfc-gh-japatel changed the base branch from master to japatel-PROD-39429-revert-param-name October 26, 2023 05:54

sfc-gh-japatel closed this Oct 26, 2023

sfc-gh-japatel reopened this Oct 26, 2023

Base automatically changed from japatel-PROD-39429-revert-param-name to master October 26, 2023 17:40

sfc-gh-japatel force-pushed the japatel-PROD-39429-http-client-get-status branch from 84e089d to 638bcc3 Compare October 26, 2023 17:48

sfc-gh-japatel added 3 commits October 26, 2023 10:49

PART-2 Make REST API(/channels/status) call which acts as Channel Exi…

32f48da

…stence Checker

Add more comments

d013d27

Add copyright

ca5b448

sfc-gh-japatel force-pushed the japatel-PROD-39429-http-client-get-status branch from 638bcc3 to ca5b448 Compare October 26, 2023 17:49

sfc-gh-tzhang reviewed Oct 27, 2023

View reviewed changes

sfc-gh-rcheng reviewed Oct 27, 2023

View reviewed changes

sfc-gh-japatel closed this Nov 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PROD-39429 PART-2 Make REST API(/channels/status) call which acts as Channel Exi… #737

PROD-39429 PART-2 Make REST API(/channels/status) call which acts as Channel Exi… #737

sfc-gh-japatel commented Oct 26, 2023 •

edited

Loading

sfc-gh-tzhang Oct 27, 2023

sfc-gh-japatel Oct 27, 2023

sfc-gh-tzhang Oct 27, 2023

sfc-gh-japatel Oct 27, 2023

sfc-gh-tzhang Oct 27, 2023

sfc-gh-tzhang Oct 27, 2023

sfc-gh-japatel Oct 27, 2023

sfc-gh-tzhang Oct 27, 2023

sfc-gh-japatel Oct 27, 2023

sfc-gh-tzhang Oct 27, 2023

sfc-gh-rcheng Oct 27, 2023

sfc-gh-rcheng Oct 27, 2023

sfc-gh-japatel Oct 27, 2023

sfc-gh-rcheng Oct 27, 2023

sfc-gh-rcheng Oct 27, 2023

sfc-gh-japatel Oct 27, 2023

	private static final String DEBUG_PREFIX = "Channel Existence Checker - Kafka Connector";
	private static final String DEBUG_PREFIX = "StreamingChannelExistenceChecker";


		ChannelExistenceCheckerResponse channelExistenceCheckerResponse;
		try (CloseableHttpResponse closeableHttpResponse = httpClient.execute(request)) {

		LOGGER.warn(
		"Null response obtained in {} for channel:{}", CHANNEL_STATUS_ENDPOINT, channelName);

PROD-39429 PART-2 Make REST API(/channels/status) call which acts as Channel Exi… #737

PROD-39429 PART-2 Make REST API(/channels/status) call which acts as Channel Exi… #737

Conversation

sfc-gh-japatel commented Oct 26, 2023 • edited Loading

What

Why

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-japatel commented Oct 26, 2023 •

edited

Loading