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: improve waiting for notifications by providing a timeout option #778

Merged
merged 12 commits into from Mar 23, 2017

Conversation

Projects
None yet
4 participants
@dmigowski
Contributor

dmigowski commented Mar 11, 2017

Improve waiting for notifications using the LISTEN/NOTIFY framework by adding an optional timeout to the PGConnection.getNotifications() method. The change does not have any effects for users that don't use this functionality and the API remain fully backwards compatible.

PS: Please squash commits when merging, the text of this pull request should give a nice commit message.

dmigowski added some commits Mar 11, 2017

Improves waiting for notifications by adding a timeout to the PGConne…
…ction.getNotifications() method. The change does not have any effects for users that don't use this functionality and the API remain fully backwards compatible.
@codecov-io

This comment has been minimized.

codecov-io commented Mar 11, 2017

Codecov Report

Merging #778 into master will increase coverage by <.01%.
The diff coverage is 58.97%.

@@             Coverage Diff              @@
##             master     #778      +/-   ##
============================================
+ Coverage     65.26%   65.27%   +<.01%     
- Complexity     3505     3519      +14     
============================================
  Files           165      165              
  Lines         15184    15219      +35     
  Branches       2456     2465       +9     
============================================
+ Hits           9910     9934      +24     
- Misses         4090     4099       +9     
- Partials       1184     1186       +2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef8c6f9...b25b008. Read the comment docs.

@vlsi

Would you please add test cases to cover the new feature?

@@ -32,6 +32,20 @@
PGNotification[] getNotifications() throws SQLException;
/**
* This method returns any notifications that have been received since the last call to this
* method. Returns null if there have been no notifications. A timeout can be speficied so the

This comment has been minimized.

@vlsi

vlsi Mar 11, 2017

Member

speficied -> specified

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 11, 2017

dmigowski added some commits Mar 11, 2017

@davecramer

This comment has been minimized.

Member

davecramer commented Mar 11, 2017

@dmigowski any chance you can roll this #579 in at the same time ?

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 12, 2017

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 12, 2017

@vlsi: I added the unit tests, also fixing a bug I found in the mean time. The code coverage is sadly -0.01% now, because i added some code I cannot get to run in the tests (the case of a Warning sent by the server while waiting for notifications). I would fix that if you can tell me how to simulate that in my tests.

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 12, 2017

Also would like to have my patch committed now. I could create a separate one for #579 if that is still wished for.

@davecramer

This comment has been minimized.

Member

davecramer commented Mar 12, 2017

looks good to me

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 14, 2017

@vlsi: What is your opinion about this commit now that I added unit tests? What about my discussion regarding your #579?

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 19, 2017

@vlsi: Can I do something for this change to be accepted? I added unit tests as requested. Please don't be insulted because I did't want to incorporate your #579 this time. Thanks in advance.

@davecramer

This comment has been minimized.

Member

davecramer commented Mar 19, 2017

@dmigowski #579 is mine, he's not insulted by it ;) I'll try to get to this Monday. Thanks!

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 19, 2017

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 20, 2017

@dmigowski , there is a couple of things that bothers me

  1. setSocketTimeout(0). It looks like it would interfere with existing socketTimeout connection option.

  2. Can you clarify why did you put all the tests into a single test method? Having separate test methods helps debugging (it is easier to execute/debug a test method rather than a part of a big test method)

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 20, 2017

dmigowski added some commits Mar 21, 2017

- Reverting old socket timeout now after using timeouts in getNotific…
…ations(timeout)

- Broke unit test into multiple methods.
@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 22, 2017

@vlsi: I broke up the test into pieces now and revert the timeout to the previous value after returing from getNotifications().

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 22, 2017

There is a failure in the replication test suite now that I didn't touch. Could someone please investigate? I don't think it is my fault.

Tests run: 56, Failures: 1, Errors: 0, Skipped: 3, Time elapsed: 7.57 sec <<< FAILURE! - in org.postgresql.replication.ReplicationTestSuite

testLastReceiveLSNCorrectOnView(org.postgresql.replication.LogicalReplicationStatusTest) Time elapsed: 0.063 sec <<< FAILURE!

java.lang.AssertionError: Replication stream by execute forceUpdateStatus should send to view actual received position that allow monitoring lag

Expected: <LSN{0/16D3390}>
but: was <LSN{0/16D3457}>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.postgresql.replication.LogicalReplicationStatusTest.testLastReceiveLSNCorrectOnView(LogicalReplicationStatusTest.java:162)

@dmigowski

This comment has been minimized.

Contributor

dmigowski commented Mar 23, 2017

@vlsi: Is there still something to do? @davecramer: Can you pull this in now?

@davecramer davecramer merged commit a7e0c83 into pgjdbc:master Mar 23, 2017

2 checks passed

codecov/project 65.27% (+<.01%) compared to ef8c6f9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vlsi vlsi added this to the 42.1.0 milestone Mar 23, 2017

davecramer added a commit to davecramer/pgjdbc that referenced this pull request Sep 19, 2017

feat: improve waiting for notifications by providing a timeout option (
…pgjdbc#778)

* Improves waiting for notifications by adding a timeout to the PGConnection.getNotifications() method. The change does not have any effects for users that don't use this functionality and the API remain fully backwards compatible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment