Skip to content

Update TestingPhoenixServer using SharedResource#16260

Merged
kokosing merged 1 commit intotrinodb:masterfrom
chenjian2664:update_phoenix_testing_server
Feb 27, 2023
Merged

Update TestingPhoenixServer using SharedResource#16260
kokosing merged 1 commit intotrinodb:masterfrom
chenjian2664:update_phoenix_testing_server

Conversation

@chenjian2664
Copy link
Copy Markdown
Contributor

Description

Update TestingPhoenixServer using SharedResource

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Feb 24, 2023
@chenjian2664 chenjian2664 self-assigned this Feb 24, 2023
Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@kokosing kokosing merged commit fc998e9 into trinodb:master Feb 27, 2023
@github-actions github-actions bot added this to the 409 milestone Feb 27, 2023
@chenjian2664 chenjian2664 deleted the update_phoenix_testing_server branch February 27, 2023 17:45
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 9, 2023

What's the benefit of this change?

@chenjian2664
Copy link
Copy Markdown
Contributor Author

What's the benefit of this change?

Improve code readability, users know to get a "Lease" from a "shared resource" instead of a new instance. Any concern from your side?

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 10, 2023

io.trino.testing.SharedResource#returnLease does reference counting and triggers shutdown.
this is good when resource can be re-creater relatively cheaply, but obviously may lead to a resource being created multiple times, perhaps once for each test class.

What's the TestingPhoenixServer startup time?

@chenjian2664
Copy link
Copy Markdown
Contributor Author

chenjian2664 commented Mar 11, 2023

Take TestingPhoenixServer for instance, returnLease is similar to the previous shutdown action, and the timing of the current call is also similar to afterClass as before. IMO there is no additional overhead for creating and closing resources compared to the previous implementation, correct me if I am wrong.

It is true that the creation of TestingPhoenixServer is time-consuming, we may be able to:

  • Implement a function similar to closeAfterClass in TestingPhoenixServer, without actively returning Lease return TestingPhoenixServer instead.
  • We can rewrite the Close method without actually closing the server, and then actively close the Lease when we want to close it.
  • A bit of a hack way :), obtain the Lease somewhere in advance, and then actively close the Lease when we want to close it.

What do you think

@kokosing
Copy link
Copy Markdown
Member

SharedResource#returnLease does reference counting and triggers shutdown.

I am under impression that previous implementation did exactly the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants