Skip to content
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: cast_primitive data type #354

Merged
merged 4 commits into from
Jun 14, 2024
Merged

Conversation

zhangweidev
Copy link
Contributor

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

函数 cast_primitive 的 data 类型转换应该是 DVAL

How do you solve it?

DTVAL -> DVAL

Special notes for your reviewer, ex. impact of this fix, design document, etc:

@CLAassistant
Copy link

CLAassistant commented Jun 11, 2024

CLA assistant check
All committers have signed the CLA.

wey-gu
wey-gu previously approved these changes Jun 12, 2024
Copy link
Contributor

@wey-gu wey-gu left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fix/contribution!

sleep 20
pdm test
- name: Test SSL connection with pytest
run: |
enable_ssl=true docker-compose -f tests/docker-compose-ssl.yaml up -d
enable_ssl=true docker compose -f tests/docker-compose-ssl.yaml up -d
Copy link
Contributor

@wey-gu wey-gu Jun 13, 2024

Choose a reason for hiding this comment

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

latest ubuntu 22.04 gh action runner comes with a different version of docker-compose, it failed here with docker-compose, but this fixed it. ref: https://askubuntu.com/questions/1508129/docker-compose-giving-containerconfig-errors-after-update-today

@wey-gu
Copy link
Contributor

wey-gu commented Jun 14, 2024

Need to look into this ci error(not related to this change)

It's caused by the new compose container naming rule, fixing with d47e871

Error response from daemon: No such container: tests_graphd0_1
Error: failed to start containers: tests_graphd0_1
FAILED
=================================== FAILURES ===================================
________________________ test_remove_invalid_connection ________________________

    def test_remove_invalid_connection():
        addresses = [("127.0.0.1", 9669), ("127.0.0.1", 9670), ("127.0.0.1", 9671)]
        configs = Config()
        configs.min_connection_pool_size = 30
        configs.max_connection_pool_size = 45
        pool = ConnectionPool()
    
        try:
            assert pool.init(addresses, configs)
    
            # turn down one server('127.0.0.1', 9669) so the connection to it is invalid
            os.system("docker stop tests_graphd0_1")
            time.sleep(3)
    
            # get connection from the pool, we should be able to still get 30 connections even though one server is down
            for i in range(0, 30):
                conn = pool.get_connection()
                assert conn is not None
    
            # total connection should still be 30
            assert pool.connects() == 30
    
            # the number of connections to the down server should be 0
>           assert len(pool._connections[addresses[0]]) == 0
E           assert 10 == 0
E            +  where 10 = len(deque([<nebula3.gclient.net.Connection.Connection object at 0x7f4debc84700>, <nebula3.gclient.net.Connection.Connection object at 0x7f4dec88cf10>, <nebula3.gclient.net.Connection.Connection object at 0x7f4dec880a30>, <nebula3.gclient.net.Connection.Connection object at 0x7f4decd3c190>, <nebula3.gclient.net.Connection.Connection object at 0x7f4decd54df0>, <nebula3.gclient.net.Connection.Connection object at 0x7f4decd54400>, ...]))

tests/test_pool.py:298: AssertionError

wey-gu
wey-gu previously approved these changes Jun 14, 2024
@@ -283,7 +283,7 @@ def test_remove_invalid_connection():
assert pool.init(addresses, configs)

# turn down one server('127.0.0.1', 9669) so the connection to it is invalid
os.system("docker stop tests_graphd0_1")
os.system("docker stop tests_graphd0_1 || docker stop tests-graphd0-1")
Copy link
Contributor

Choose a reason for hiding this comment

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

The new docker-compose comes with a different naming per container...

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.27%. Comparing base (6672e51) to head (c21b05e).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #354      +/-   ##
==========================================
- Coverage   75.35%   75.27%   -0.08%     
==========================================
  Files          19       19              
  Lines        2710     2710              
==========================================
- Hits         2042     2040       -2     
- Misses        668      670       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wey-gu wey-gu requested review from Nicole00 and BeautyyuYanli June 14, 2024 08:38
@wey-gu
Copy link
Contributor

wey-gu commented Jun 14, 2024

@Nicole00

Only one line of change, other changes are due to github action runner change, caused docker-compose broken in Ubuntu 22.04.

@Nicole00 Nicole00 merged commit 5d562fb into vesoft-inc:master Jun 14, 2024
10 checks passed
wey-gu added a commit that referenced this pull request Jul 16, 2024
* fix: cast_primitive data type

* ci: fix modern OS's docker compose version

ref: https://askubuntu.com/questions/1508129/docker-compose-giving-containerconfig-errors-after-update-today

* ci: adapt new docker compose naming

* make linter happy

---------

Co-authored-by: Wey Gu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants