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 #78 use space and gql are executed together incorrect in 3.3.0 #87

Merged
merged 4 commits into from
Nov 25, 2022

Conversation

CorvusYe
Copy link
Collaborator

@CorvusYe CorvusYe commented Nov 7, 2022

@llinzhe
When switch space is required, will execute twice ,
one is USE space , another is normal nGQL

@CorvusYe CorvusYe requested a review from wey-gu November 7, 2022 15:30
@llinzhe
Copy link

llinzhe commented Nov 7, 2022

@llinzhe When switch space is required, will execute twice , one is USE space , another is normal nGQL

Great feeling for your prompt response and repair!

@CorvusYe
Copy link
Collaborator Author

CorvusYe commented Nov 7, 2022

May need your help with further testing. I'm using 3.2.0 now.

@CorvusYe
Copy link
Collaborator Author

CorvusYe commented Nov 7, 2022

If things go well, the merge will automatically release a snapshot version tomorrow.

@llinzhe
Copy link

llinzhe commented Nov 7, 2022

If things go well, the merge will automatically release a snapshot version tomorrow.
I'll test it with a snapshot version, in the nebula 3.3 environment

if (!sameSpace) {
qlAndSpace[0] = currentSpace;
Session session = localSession.getSession();
session.execute(String.format("USE %s", currentSpace));
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to execute USE space after each request. In resultSet of execution's response, there's space name for current session, and we can decide whether need to change the space back.

If execute USE space before request, there will be some useless operations, ge:
Local session's current space is test, and user's request is "use test1; show hosts; use test" , and there is no need to pre-execute a use test1 operation.

Copy link
Collaborator Author

@CorvusYe CorvusYe Nov 8, 2022

Choose a reason for hiding this comment

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

In this case, you are right.

And we should consider another situation, for example there are two DAO:

<mapper namespace="Test1Dao" space="test1">

    <select id="useCurrentSpace" >
         RETURN 'useCurrentSpace';
    </select>

    <select id="useTestSpace" space="test">
         RETURN 'useTestSpace';
    </select>

    <select id="useSpaceInner">
         USE test;
         RETURN 'useSpaceInner';
    </select>
    
</mapper>


<mapper namespace="DefaultSpaceInYmlDao"> <!-- default space is `test`-->
    
    <select id="normalInvoke">
         RETURN 'normalInvoke';
    </select>

</mapper>
    @Autowired private Test1Dao test1Dao;
    @Autowired private DefaultSpaceInYmlDao defaultSpaceDao;
    
    public void biz() {
        test1Dao.useCurrentSpace(); // when this session is use in first time, space is null, so need to specify the space
        // USE test1;
        // RETURN 'useCurrentSpace';

        test1Dao.useCurrentSpace(); // local session keep the last space, so this time is unnecessary to switch
        // RETURN 'useCurrentSpace';

        test1Dao.useTestSpace(); // catching the space is difference from local session.
        // USE test;
        // RETURN 'useTestSpace';

        defaultSpaceDao.normalInvoke(); 
        // RETURN 'normalInvoke';

        test1Dao.useTestSpace();
        // RETURN 'useTestSpace';

        test1Dao.useSpaceInner(); // This usage #78 may happend again, because we can not control how ngql will be written
        // USE test; RETURN 'useSpaceInner'; 
    }

Let's assume that only one session is in use.
In this case 6 times invoked, we costs 8 times request. not 12 times.

Ngbatis recommends declaring spaces in xml tags.

And you reminded me that we can get current session space from ResultSet insteads of RegExp.

@llinzhe
Copy link

llinzhe commented Nov 9, 2022

May need your help with further testing. I'm using 3.2.0 now.

I passed the test in 3.3.0 environment

@CorvusYe
Copy link
Collaborator Author

CorvusYe commented Nov 9, 2022

May need your help with further testing. I'm using 3.2.0 now.

I passed the test in 3.3.0 environment

Thanks a lot.

@CorvusYe CorvusYe merged commit f0cb200 into nebula-contrib:master Nov 25, 2022
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.

4 participants