-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[fix] [connector-tdengine] fix sql exception and concurrentmodifyexception when connect to taos and read data #6088
[fix] [connector-tdengine] fix sql exception and concurrentmodifyexception when connect to taos and read data #6088
Conversation
// signal to the source that we have reached the end of the data. | ||
log.info("Closed the bounded TDengine source"); | ||
context.signalNoMoreElement(); | ||
log.info("polling new split from queue!"); |
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.
PTAL @hailin0
please try merge from dev. |
@Hisoka-X checked out dev and still got same error |
Are you try rebuild source code? Because before run e2e, seatunnel need build to refresh jar. |
@Hisoka-X I rebuild project and still got same error command: mvn package -pl seatunnel-dist -am -Dmaven.test.skip=true |
Your commit list needs to be cleaned up |
6c96d24
to
cfe371a
Compare
@hailin0 PTAL |
Can you provide some test cases? |
@liugddx done |
} | ||
} else if (noMoreSplit && sourceSplits.isEmpty()) { | ||
// signal to the source that we have reached the end of the data. | ||
log.info("Closed the bounded jdbc source"); |
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.
It is more appropriate to replace jdbc source with TDengine source.
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 don't quite understand what you mean~
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 mean the log print content may need to be adjusted.
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 mean the log print content may need to be adjusted.
hah~ get it!
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.
3a92d00
to
a7eb663
Compare
Please fix ci. |
eeeab0a
to
e5feb0a
Compare
1. fix no suitable driver found exception while connecting to taos by jdbc 2. print cause when throw TDengineConnectorException 3. fix concurrentmodifyexception when operating splits
test poll and add split in reader
e5feb0a
to
ed27f84
Compare
properties.put(TSDBDriver.PROPERTY_KEY_USER, config.getUsername()); | ||
properties.put(TSDBDriver.PROPERTY_KEY_PASSWORD, config.getPassword()); |
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.
Can other jdbc parameters be added here?
StringUtils.join( | ||
config.getUrl(), | ||
config.getDatabase(), | ||
"?user=", |
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.
why update this config?
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.
why update this config?
It's not safe to put username and password in jdbc url, it's better to put password in properties and pass properties to jdbc driver in production
There's a risk that the JDBC URL, including the password, could be logged inadvertently, especially if the logging level is not properly configured to exclude sensitive information.
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.
@alextinng here
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.
How did you fix this?
No suitable driver found for jdbc:TAOS-RS://localhost:6041/test?user=root&password=taosdata
already fixed in previous commit by others, see line: 134-135 |
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.
LGTM
Purpose of this pull request
close #6032
close #5998
Does this PR introduce any user-facing change?
no
How was this patch tested?
test with seatunnel-engine-example, belowing is application log:
Check list
New License Guide
release-note
.