-
Notifications
You must be signed in to change notification settings - Fork 99
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
PROD-39429 Implement migrate sys func from new channel(Format V2) to old channel (V1) - Push to Main #751
PROD-39429 Implement migrate sys func from new channel(Format V2) to old channel (V1) - Push to Main #751
Changes from 4 commits
cc968f3
a14b7da
c1cbaa0
9c6783b
62100c1
612027b
5962624
b7b5ab5
3dbf6c1
21697ad
e54b031
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,10 @@ | |
import static com.snowflake.kafka.connector.Utils.TABLE_COLUMN_CONTENT; | ||
import static com.snowflake.kafka.connector.Utils.TABLE_COLUMN_METADATA; | ||
|
||
import com.fasterxml.jackson.core.JsonProcessingException; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import com.snowflake.kafka.connector.Utils; | ||
import com.snowflake.kafka.connector.internal.streaming.ChannelMigrateOffsetTokenResponseDTO; | ||
import com.snowflake.kafka.connector.internal.streaming.IngestionMethodConfig; | ||
import com.snowflake.kafka.connector.internal.streaming.SchematizationUtils; | ||
import com.snowflake.kafka.connector.internal.telemetry.SnowflakeTelemetryService; | ||
|
@@ -56,6 +59,8 @@ public class SnowflakeConnectionServiceV1 implements SnowflakeConnectionService | |
// User agent suffix we want to pass in to ingest service | ||
public static final String USER_AGENT_SUFFIX_FORMAT = "SFKafkaConnector/%s provider/%s"; | ||
|
||
private final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); | ||
|
||
SnowflakeConnectionServiceV1( | ||
Properties prop, | ||
SnowflakeURL url, | ||
|
@@ -1006,4 +1011,60 @@ public Connection getConnection() { | |
public SnowflakeInternalStage getInternalStage() { | ||
return this.internalStage; | ||
} | ||
|
||
@Override | ||
public boolean migrateStreamingChannelOffsetToken( | ||
String tableName, String sourceChannelName, String destinationChannelName) { | ||
InternalUtils.assertNotEmpty("tableName", tableName); | ||
InternalUtils.assertNotEmpty("sourceChannelName", sourceChannelName); | ||
InternalUtils.assertNotEmpty("destinationChannelName", destinationChannelName); | ||
String fullyQualifiedTableName = | ||
prop.getProperty(InternalUtils.JDBC_DATABASE) | ||
+ "." | ||
+ prop.getProperty(InternalUtils.JDBC_SCHEMA) | ||
+ "." | ||
+ tableName; | ||
String query = "select SYSTEM$SNOWPIPE_STREAMING_MIGRATE_CHANNEL_OFFSET_TOKEN((?), (?), (?));"; | ||
|
||
try { | ||
PreparedStatement stmt = conn.prepareStatement(query); | ||
stmt.setString(1, fullyQualifiedTableName); | ||
stmt.setString(2, sourceChannelName); | ||
stmt.setString(3, destinationChannelName); | ||
ResultSet resultSet = stmt.executeQuery(); | ||
|
||
String migrateOffsetTokenResultFromSysFunc = null; | ||
if (resultSet.next()) { | ||
migrateOffsetTokenResultFromSysFunc = resultSet.getString(1 /*Only one column*/); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. out of scope for this PR - do we have any guarantees that this DTO won't change on the server side? a comment, test or something server side to warn against changes since we now throw the exception on failure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, i think the json exception will help with that.. I think I have the comment on server side saying be cautious to make changes in the interface. but let me try to mimick and see what happens if response is changed. I could probably handle new fields in the object mapper. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved the method to its own method so that we can test it. If the response changes from server side which shouldnt, this tests methods will start failing |
||
} | ||
if (migrateOffsetTokenResultFromSysFunc == null) { | ||
LOGGER.warn( | ||
"No result found in Migrating OffsetToken through System Function for tableName:{}," | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does it mean by no result found? No destination channel or source channel found? or both? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is not expected, should we throw an exception? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not expected and I thought we decided to not throw any exceptions/swallow all and continue using old channel. I can see the concern though but at this point I feel it is about whether we want to halt ingestion or atleast ignore the exception and continue moving forward with old. Halting ingestion could be better and it helps us know something is wrong rather than continuing with old channel with ramifications we dont. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that its better to swallow this exception here, but we should have something to track an error here - maybe an alert on the server side if we don't return an offset on the func call, or a new telemetry event similar to reportKafkaFatalError (if we aren't expecting too many hits on this error) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, both.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For an unexpected exception, I am throwing a runtime exception. PTAL! It's better to fail here IMO. Thanks folks! |
||
+ " sourceChannel:{}, destinationChannel:{}", | ||
fullyQualifiedTableName, | ||
sourceChannelName, | ||
destinationChannelName); | ||
return false; | ||
} | ||
|
||
ChannelMigrateOffsetTokenResponseDTO channelMigrateOffsetTokenResponseDTO = | ||
OBJECT_MAPPER.readValue( | ||
migrateOffsetTokenResultFromSysFunc, ChannelMigrateOffsetTokenResponseDTO.class); | ||
LOGGER.info( | ||
"Migrate OffsetToken response for table:{}, sourceChannel:{}, destinationChannel:{}" | ||
+ " is:{}", | ||
tableName, | ||
sourceChannelName, | ||
destinationChannelName, | ||
channelMigrateOffsetTokenResponseDTO); | ||
Comment on lines
+1054
to
+1060
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This log might be confusing to customer, could we only log when there is actually a migration being done? Or do we even need this client side log given that we have a bunch of server side logging in place? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be fine, this is only once per channel during open partition, not very often. I would like to keep this which helps us in debugging any customer issue. |
||
return true; | ||
} catch (SQLException | JsonProcessingException e) { | ||
LOGGER.error( | ||
sfc-gh-japatel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"Migrating OffsetToken for a SourceChannel:{} in table:{} failed due to:{}", | ||
sourceChannelName, | ||
fullyQualifiedTableName, | ||
e.getMessage()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getMessage might be NULL for some exceptions, could we do better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be worth logging stacktrace too. let me add that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
return false; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
package com.snowflake.kafka.connector.internal.streaming; | ||
sfc-gh-japatel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
||
/** | ||
* POJO used to serialize the System function response for migration offset from Source Channel to | ||
* Destination. | ||
*/ | ||
public class ChannelMigrateOffsetTokenResponseDTO { | ||
private long responseCode; | ||
|
||
private String responseMessage; | ||
|
||
public ChannelMigrateOffsetTokenResponseDTO(long responseCode, String responseMessage) { | ||
this.responseCode = responseCode; | ||
this.responseMessage = responseMessage; | ||
} | ||
|
||
/** Default Ctor for Jackson */ | ||
public ChannelMigrateOffsetTokenResponseDTO() {} | ||
|
||
@JsonProperty("responseCode") | ||
public long getResponseCode() { | ||
return responseCode; | ||
} | ||
|
||
@JsonProperty("responseMessage") | ||
public String getResponseMessage() { | ||
return responseMessage; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return "ChannelMigrateOffsetTokenResponseDTO{" | ||
+ "responseCode=" | ||
+ responseCode | ||
+ ", responseMessage='" | ||
+ responseMessage | ||
+ '\'' | ||
+ '}'; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: lets add something about how V2 is deprecated. Otherwise customers may disable this because V2 sounds fancier than V1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, let me add!