-
Notifications
You must be signed in to change notification settings - Fork 211
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
Initial PR for stream support for Postgres in Rds source #5310
base: main
Are you sure you want to change the base?
Changes from 5 commits
3767b1f
5f72e59
9381480
4fbdbdb
9e70372
81c5883
a3bec6f
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 |
---|---|---|
|
@@ -16,6 +16,12 @@ public class StreamProgressState { | |
@JsonProperty("currentPosition") | ||
private BinlogCoordinate currentPosition; | ||
|
||
@JsonProperty("currentLsn") | ||
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. Consider creating new class to separate MySQL vs Postgres properties. 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 model is used in serialization/deserialization by the source coordinator, it's a bit more involved to separate into two classes. I added a TODO in code and will address it in followup PRs. |
||
private String currentLsn; | ||
|
||
@JsonProperty("replicationSlotName") | ||
private String replicationSlotName; | ||
|
||
@JsonProperty("waitForExport") | ||
private boolean waitForExport = false; | ||
|
||
|
@@ -26,10 +32,22 @@ public BinlogCoordinate getCurrentPosition() { | |
return currentPosition; | ||
} | ||
|
||
public String getCurrentLsn() { | ||
return currentLsn; | ||
} | ||
|
||
public String getReplicationSlotName() { | ||
return replicationSlotName; | ||
} | ||
|
||
public void setCurrentPosition(BinlogCoordinate currentPosition) { | ||
this.currentPosition = currentPosition; | ||
} | ||
|
||
public void setReplicationSlotName(String replicationSlotName) { | ||
this.replicationSlotName = replicationSlotName; | ||
} | ||
|
||
public boolean shouldWaitForExport() { | ||
return waitForExport; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
package org.opensearch.dataprepper.plugins.source.rds.datatype.postgres; | ||
|
||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
public enum ColumnType { | ||
BOOLEAN(16, "boolean"), | ||
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. Are those typeId from postgres? Is there way we can use their SDK variables instead of the hardcoded number 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. Yes, they are from postgres, but I don't think they have those in their libraries. Postgres has a system table |
||
SMALLINT(21, "smallint"), | ||
INTEGER(23, "integer"), | ||
BIGINT(20, "bigint"), | ||
REAL(700, "real"), | ||
DOUBLE_PRECISION(701, "double precision"), | ||
NUMERIC(1700, "numeric"), | ||
TEXT(25, "text"), | ||
VARCHAR(1043, "varchar"), | ||
DATE(1082, "date"), | ||
TIME(1083, "time"), | ||
TIMESTAMP(1114, "timestamp"), | ||
TIMESTAMPTZ(1184, "timestamptz"), | ||
UUID(2950, "uuid"), | ||
JSON(114, "json"), | ||
JSONB(3802, "jsonb"); | ||
|
||
private final int typeId; | ||
private final String typeName; | ||
|
||
private static final Map<Integer, ColumnType> TYPE_ID_MAP = new HashMap<>(); | ||
|
||
static { | ||
for (ColumnType type : values()) { | ||
TYPE_ID_MAP.put(type.typeId, type); | ||
} | ||
} | ||
|
||
ColumnType(int typeId, String typeName) { | ||
this.typeId = typeId; | ||
this.typeName = typeName; | ||
} | ||
|
||
public int getTypeId() { | ||
return typeId; | ||
} | ||
|
||
public String getTypeName() { | ||
return typeName; | ||
} | ||
|
||
public static ColumnType getByTypeId(int typeId) { | ||
return TYPE_ID_MAP.get(typeId); | ||
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. Add exception handling for typeId |
||
} | ||
|
||
public static String getTypeNameByEnum(ColumnType columnType) { | ||
return columnType.getTypeName(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ | |
import java.sql.SQLException; | ||
import java.util.Properties; | ||
|
||
public class ConnectionManager { | ||
public class MySqlConnectionManager { | ||
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. Consider creating ConnectionManager interface and implementation for MySql/Postgres ConnectionManager 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. Created the interface. |
||
static final String JDBC_URL_FORMAT = "jdbc:mysql://%s:%d"; | ||
static final String USERNAME_KEY = "user"; | ||
static final String PASSWORD_KEY = "password"; | ||
|
@@ -25,7 +25,7 @@ public class ConnectionManager { | |
private final String password; | ||
private final boolean requireSSL; | ||
|
||
public ConnectionManager(String hostName, int port, String username, String password, boolean requireSSL) { | ||
public MySqlConnectionManager(String hostName, int port, String username, String password, boolean requireSSL) { | ||
this.hostName = hostName; | ||
this.port = port; | ||
this.username = username; | ||
|
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.
This class needs some refactor. We should follow single responsibility principle here. This class has dependency on MySQL and Postgres Client. We should move these dependencies to separate class.
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.
I did some refactoring and extracted out the creation of schemaManagers in separate classes. The
ReplicationLogClientFactory
already hides the creation of MySQL/Postgres clients.